Merge cockroachdb#154467 cockroachdb#156547 cockroachdb#156953 cockroachdb#156987

154467: roachprod: azure GetVMSpecs() implementation r=DarrylWong a=williamchoe3

Informs cockroachdb#146202 

Implementation for previously unimplemented interface method `GetVMSpecs()` for Azure. This will allow for cluster information to be fetched in roachtest via `FetchVMSpecs`

Also changed `azureIDPattern` to match the optional type name pairs that may exist in a fully qualified azure resource id. See comments for more details.  Previously, if these optional type name pairs existed, they were just a part of the `resourceName` field. Currently, I didn't see any of these child resource ids being parsed, but if we were to ever parse those in the future the previous implementation wouldn't make sense.
* Note: Although the regex selects child resource type and names in the last matching group, they are not saved to AzureId struct because they are not being used

For Azure, `client.Get(...)` allows you to pass in an optional `expander` parameter which seems to provide additional information for `"instanceView"` and runs an optionally passed in user script (passed in on vm creation) when `"userData"` is passed in.
*  `"userData"` doesn't seem relevant for us "retrieves the UserData property as part of the VM model view that was provided by the user during the VM Create/Update operation" https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/get?view=rest-compute-2025-04-01&tabs=HTTP
  *  "The `instanceView` of an Azure Virtual Machine (VM) provides a snapshot of the runtime properties and status of the VM" https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/instance-view?view=rest-compute-2025-04-01&tabs=HTTP Seems nice, but there's no diff in the json returned by `"instanceView"` and the default when `expander` is not set (referred to `model view` in some places on thier docs) 
```go
InstanceViewTypesInstanceView InstanceViewTypes = "instanceView"  
InstanceViewTypesUserData InstanceViewTypes = "userData"
```


IBM PR: cockroachdb#155368


156547: sql: add ColumnGeneratedAsIdentity as a new schema change element r=shghasemi a=shghasemi

Previously, GeneratedAsIdentity was part of the Column element.
Creating a new schema change element will allow the declarative schema changer
to add/remove/update such columns without dropping the Column element.

Epic: [CRDB-31283](https://cockroachlabs.atlassian.net/browse/CRDB-31283)

Part of: cockroachdb#142918

Release note: None

156953: kvclient/kvcoord: remove stale TODO in Step r=yuzefovich a=yuzefovich

We currently have a TODO in Step to add an assertion that stepping is only done on the RootTxn which references a now-closed issue. I did a little bit archeology, and in bd255e5 we consciously removed the assertion like this since stepping on a LeafTxn should be no-op, should just work, and is actually needed behavior for internal executors when they run on remote nodes as part of DistSQL plan. (In connExecutor we currently unconditionally Step the txn on the main query path.) Thus I don't think we want to implement the TODO and should just remove it.

Epic: None
Release note: None

156987: sql: remove now-stale growstack for internal executor r=yuzefovich a=yuzefovich

As of 4a5c503, we now grow stack unconditionally for all async tasks. Some time ago we added an ability to growstack in the async task of the internal executor when used in the LDR, and now that has become redundant, so we remove this special case.

Epic: None
Release note: None

Co-authored-by: William Choe <[email protected]>
Co-authored-by: Shadi Ghasemitaheri <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: 4 people

pkg/cmd/roachtest/tests/roachtest.go

@@ -81,6 +81,18 @@ func registerRoachtest(r registry.Registry) {
 			monitorFatalTestGlobal(ctx, t, c)
 		},
 	})
+
+	// Manual test for verifying framework behavior in a test success scenario
+	r.Add(registry.TestSpec{
+		Name:             "roachtest/manual/success",
+		Owner:            registry.OwnerTestEng,
+		Cluster:          r.MakeClusterSpec(3),
+		CompatibleClouds: registry.AllClouds,
+		Suites:           registry.ManualOnly,
+		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
+			t.L().Printf("hello")
+		},
+	})
 }
 
 // monitorFatalTest will always fail with a node logging a fatal error in a

pkg/crosscluster/logical/lww_row_processor.go

@@ -407,10 +407,6 @@ var (
 		// Use generic query plans since our queries are extremely simple and
 		// won't benefit from custom plans.
 		PlanCacheMode: &forceGenericPlan,
-		// We've observed in the CPU profiles that the default goroutine stack
-		// of the connExecutor goroutine is insufficient for evaluation of the
-		// ingestion queries, so we grow the stack right away to 32KiB.
-		GrowStackSize: true,
 		// We don't get any benefits from generating plan gists for internal
 		// queries, so we disable them.
 		DisablePlanGists:      true,

pkg/kv/kvclient/kvcoord/txn_coord_sender.go

@@ -1525,11 +1525,6 @@ func (tc *TxnCoordSender) TestingCloneTxn() *roachpb.Transaction {
 
 // Step is part of the TxnSender interface.
 func (tc *TxnCoordSender) Step(ctx context.Context, allowReadTimestampStep bool) error {
-	// TODO(nvanbenschoten): it should be possible to make this assertion, but
-	// the API is currently misused by the connExecutor. See #86162.
-	// if tc.typ != kv.RootTxn {
-	//	return errors.AssertionFailedf("cannot step in non-root txn")
-	// }
 	tc.mu.Lock()
 	defer tc.mu.Unlock()
 	if allowReadTimestampStep && tc.shouldStepReadTimestampLocked() {

pkg/roachprod/vm/azure/BUILD.bazel

@@ -41,7 +41,10 @@ go_library(
 
 go_test(
     name = "azure_test",
-    srcs = ["utils_test.go"],
+    srcs = [
+        "ids_test.go",
+        "utils_test.go",
+    ],
     data = glob(["testdata/**"]),
     embed = [":azure"],
     deps = [

pkg/roachprod/vm/azure/azure.go

@@ -210,10 +210,49 @@ func (p *Provider) GetLiveMigrationVMs(
 	return liveMigrationVMs, nil
 }
 
+// GetVMSpecs implements the vm.GetVMSpecs interface method which returns a
+// map from VM.Name to a map of VM attributes
 func (p *Provider) GetVMSpecs(
 	l *logger.Logger, vms vm.List,
 ) (map[string]map[string]interface{}, error) {
-	return nil, nil
+	ctx, cancel := context.WithTimeout(context.Background(), p.OperationTimeout)
+	defer cancel()
+	sub, err := p.getSubscription(ctx)
+	if err != nil {
+		return nil, err
+	}
+	client := compute.NewVirtualMachinesClient(sub)
+	if client.Authorizer, err = p.getAuthorizer(); err != nil {
+		return nil, err
+	}
+
+	// Extract the spec of all VMs and create a map from VM name to spec.
+	vmSpecs := make(map[string]map[string]interface{})
+	for _, vmInstance := range vms {
+		if vmInstance.ProviderID == "" {
+			return nil, errors.Errorf("provider id not found for vm: %s", vmInstance.Name)
+		}
+		azureVmId, err := parseAzureID(vmInstance.ProviderID)
+		if err != nil {
+			return nil, err
+		}
+		l.Printf("Getting VM Specs for VM: %s", vmInstance.Name)
+		azureVm, err := client.Get(ctx, azureVmId.resourceGroup, azureVmId.resourceName, "")
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to get vm information for vm %s", vmInstance.Name)
+		}
+		// Marshaling & unmarshalling struct to match interface method return type
+		rawJSON, err := azureVm.MarshalJSON()
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to marshal vm information for vm %s", vmInstance.Name)
+		}
+		var vmSpec map[string]interface{}
+		if err := json.Unmarshal(rawJSON, &vmSpec); err != nil {
+			return nil, errors.Wrapf(err, "failed to parse raw json")
+		}
+		vmSpecs[vmInstance.Name] = vmSpec
+	}
+	return vmSpecs, nil
 }
 
 func (p *Provider) CreateVolumeSnapshot(

pkg/roachprod/vm/azure/ids.go

@@ -20,9 +20,35 @@ import (
 // https://github.com/Azure/azure-sdk-for-go/issues/3080
 // is solved.
 
+// azureIDPattern matches
+// /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{namespace}/{type}/{name}[/childType/childName ...]
+// If child resource type name pair(s) are not present, the last matching
+// group will be empty. Otherwise, the last matching group will contain all
+// additional pairs.
+//
+// Currently, we are not using any Azure Resource IDs that contain child
+// resource type name pairs, so we do not save them as a field in azureID. If
+// we need to use them in the future we can add a field to azureID and set
+// that field in parseAzureID.
+// Note: the number of child resource pairs is ambiguous and currently the
+// entire remainder of the string gets matched to the last group if a trailing
+// '/' is present after resourceName.
 var azureIDPattern = regexp.MustCompile(
-	"/subscriptions/(.+)/resourceGroups/(.+)/providers/(.+?)/(.+?)/(.+)")
+	`/subscriptions/([^/]+)/resourceGroups/([^/]+)/providers/([^/]+)/([^/]+)/([^/]+)(?:/(.*))?`)
 
+// azureID defines a fully qualified Azure Resource ID which must contain a
+// subscription ID, a resourceGroup name, a provider namespace, and at least
+// one type and name pair. The first type name pair describes the resourceType
+// and resourceName. For examples, reference unit tests.
+//
+// Additional type name pairs are optional, and if they exist, this Resource ID
+// is describing a child resource or sometimes referred to as a subresource.
+// This struct does not contain a field for child resources. If a
+// child resource ID needs to be parsed, this struct must be extended.
+//
+// Template:
+//
+//	/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{namespace}/{type}/{name}[/childType/childName ...]
 type azureID struct {
 	provider      string
 	resourceGroup string
@@ -31,14 +57,17 @@ type azureID struct {
 	subscription  string
 }
 
+// String does not account for child resources since they are not saved after
+// being parsed
 func (id azureID) String() string {
-	return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/%s/%s/%s",
+	s := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/%s/%s/%s",
 		id.subscription, id.resourceGroup, id.provider, id.resourceType, id.resourceName)
+	return s
 }
 
 func parseAzureID(id string) (azureID, error) {
 	parts := azureIDPattern.FindStringSubmatch(id)
-	if len(parts) == 0 {
+	if len(parts) != 7 {
 		return azureID{}, errors.Errorf("could not parse Azure ID %q", id)
 	}
 	ret := azureID{

pkg/roachprod/vm/azure/ids_test.go

@@ -0,0 +1,103 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package azure
+
+import "testing"
+
+func TestParseAzureID(t *testing.T) {
+	tests := []struct {
+		name           string
+		input          string
+		expectedString string
+		expectErr      bool
+		expected       azureID
+	}{
+		{
+			name:           "Valid VM",
+			input:          "/subscriptions/1234-abcd/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachines/n01",
+			expectedString: "/subscriptions/1234-abcd/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachines/n01",
+			expected: azureID{
+				subscription:  "1234-abcd",
+				resourceGroup: "my-rg",
+				provider:      "Microsoft.Compute",
+				resourceType:  "virtualMachines",
+				resourceName:  "n01",
+			},
+		},
+		{
+			name:           "Valid NIC",
+			input:          "/subscriptions/1234-abcd/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/test-nic01",
+			expectedString: "/subscriptions/1234-abcd/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/test-nic01",
+			expected: azureID{
+				subscription:  "1234-abcd",
+				resourceGroup: "test-rg",
+				provider:      "Microsoft.Network",
+				resourceType:  "networkInterfaces",
+				resourceName:  "test-nic01",
+			},
+		},
+		{
+			// Since child resources are not implemented, all we are checking here is
+			// that the resourceName gets selected out properly
+			name:           "Valid with child resource",
+			input:          "/subscriptions/1111/resourceGroups/rg1/providers/Microsoft.Network/loadBalancers/lb01/frontendIPConfigurations/ipconfig1",
+			expectedString: "/subscriptions/1111/resourceGroups/rg1/providers/Microsoft.Network/loadBalancers/lb01",
+			expected: azureID{
+				subscription:  "1111",
+				resourceGroup: "rg1",
+				provider:      "Microsoft.Network",
+				resourceType:  "loadBalancers",
+				resourceName:  "lb01",
+			},
+		},
+		{
+			name:      "Invalid - missing fields",
+			input:     "/subscriptions/abcd/resourceGroups//providers/Microsoft.Compute/virtualMachines/vm1",
+			expectErr: true,
+		},
+		{
+			name:      "Invalid - not an Azure ID",
+			input:     "/this/is/not/an/azure/id",
+			expectErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			actual, err := parseAzureID(tt.input)
+			if tt.expectErr {
+				if err == nil {
+					t.Errorf("expected error but got none")
+				}
+				return
+			}
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+				return
+			}
+			// Compare fields
+			if actual.subscription != tt.expected.subscription {
+				t.Errorf("subscription: actual %q, expected %q", actual.subscription, tt.expected.subscription)
+			}
+			if actual.resourceGroup != tt.expected.resourceGroup {
+				t.Errorf("resourceGroup: actual %q, expected %q", actual.resourceGroup, tt.expected.resourceGroup)
+			}
+			if actual.provider != tt.expected.provider {
+				t.Errorf("provider: actual %q, expected %q", actual.provider, tt.expected.provider)
+			}
+			if actual.resourceType != tt.expected.resourceType {
+				t.Errorf("resourceType: actual %q, expected %q", actual.resourceType, tt.expected.resourceType)
+			}
+			if actual.resourceName != tt.expected.resourceName {
+				t.Errorf("resourceName: actual %q, expected %q", actual.resourceName, tt.expected.resourceName)
+			}
+			// Ensure String() reconstructs the same input
+			if actual.String() != tt.expectedString {
+				t.Errorf("String(): actual %q, expected %q", actual.String(), tt.expectedString)
+			}
+		})
+	}
+}

pkg/sql/BUILD.bazel

@@ -575,7 +575,6 @@ go_library(
         "//pkg/util/errorutil",
         "//pkg/util/errorutil/unimplemented",
         "//pkg/util/fsm",
-        "//pkg/util/growstack",
         "//pkg/util/grpcutil",
         "//pkg/util/grunning",
         "//pkg/util/hlc",

pkg/sql/internal.go

@@ -39,7 +39,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/fsm"
-	"github.com/cockroachdb/cockroach/pkg/util/growstack"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -223,7 +222,6 @@ func (ie *InternalExecutor) runWithEx(
 	syncCallback func([]*streamingCommandResult),
 	errCallback func(error),
 	attributeToUser bool,
-	growStackSize bool,
 ) error {
 	ex, err := ie.initConnEx(ctx, txn, w, mode, sd, stmtBuf, syncCallback, attributeToUser)
 	if err != nil {
@@ -251,11 +249,6 @@ func (ie *InternalExecutor) runWithEx(
 	go func() {
 		defer hdl.Activate(ctx).Release(ctx)
 		defer cleanup(ctx)
-		// TODO(yuzefovich): benchmark whether we should be growing the
-		// stack size unconditionally.
-		if growStackSize {
-			growstack.Grow()
-		}
 		if err := ex.run(
 			ctx,
 			ie.mon,
@@ -1196,7 +1189,6 @@ func (ie *InternalExecutor) execInternal(
 		txn.SetBufferedWritesEnabled(false)
 	}
 	attributeToUser := sessionDataOverride.AttributeToUser && attributeToUserEnabled.Get(&ie.s.cfg.Settings.SV)
-	growStackSize := sessionDataOverride.GrowStackSize
 	if !rw.async() && (txn != nil && txn.Type() == kv.RootTxn) {
 		// If the "outer" query uses the RootTxn and the sync result channel is
 		// requested, then we must disable both DistSQL and Streamer to ensure
@@ -1308,7 +1300,7 @@ func (ie *InternalExecutor) execInternal(
 	errCallback := func(err error) {
 		_ = rw.addResult(ctx, ieIteratorResult{err: err})
 	}
-	err = ie.runWithEx(ctx, opName, txn, rw, mode, sd, stmtBuf, &wg, syncCallback, errCallback, attributeToUser, growStackSize)
+	err = ie.runWithEx(ctx, opName, txn, rw, mode, sd, stmtBuf, &wg, syncCallback, errCallback, attributeToUser)
 	if err != nil {
 		return nil, err
 	}

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go

@@ -116,11 +116,10 @@ func alterTableAddColumn(
 	spec := addColumnSpec{
 		tbl: tbl,
 		col: &scpb.Column{
-			TableID:                 tbl.TableID,
-			ColumnID:                desc.ID,
-			IsHidden:                desc.Hidden,
-			IsInaccessible:          desc.Inaccessible,
-			GeneratedAsIdentityType: desc.GeneratedAsIdentityType,
+			TableID:        tbl.TableID,
+			ColumnID:       desc.ID,
+			IsHidden:       desc.Hidden,
+			IsInaccessible: desc.Inaccessible,
 		},
 		unique:  d.Unique.IsUnique,
 		notNull: !desc.Nullable,
@@ -139,9 +138,6 @@ func alterTableAddColumn(
 	if pgAttNum := desc.GetPGAttributeNum(); pgAttNum != catid.PGAttributeNum(desc.ID) {
 		spec.col.PgAttributeNum = pgAttNum
 	}
-	if ptr := desc.GeneratedAsIdentitySequenceOption; ptr != nil {
-		spec.col.GeneratedAsIdentitySequenceOption = *ptr
-	}
 	spec.name = &scpb.ColumnName{
 		TableID:  tbl.TableID,
 		ColumnID: spec.col.ColumnID,
@@ -290,6 +286,26 @@ func alterTableAddColumn(
 		}
 		b.IncrementSchemaChangeAddColumnQualificationCounter("on_update")
 	}
+	if d.GeneratedIdentity.IsGeneratedAsIdentity {
+		generatedAsIdentityType := desc.GeneratedAsIdentityType
+		seqOptions := ""
+		if ptr := desc.GeneratedAsIdentitySequenceOption; ptr != nil {
+			seqOptions = *ptr
+		}
+		// Versions from 26.1 GeneratedAsIdentity will have a separate element for
+		// GeneratedAsIdentity. Older versios store it in the column element.
+		if spec.colType.ElementCreationMetadata.In_26_1OrLater {
+			spec.generatedAsID = &scpb.ColumnGeneratedAsIdentity{
+				TableID:        tbl.TableID,
+				ColumnID:       spec.col.ColumnID,
+				Type:           generatedAsIdentityType,
+				SequenceOption: seqOptions,
+			}
+		} else {
+			spec.col.GeneratedAsIdentityType = generatedAsIdentityType
+			spec.col.GeneratedAsIdentitySequenceOption = seqOptions
+		}
+	}
 	// Add secondary indexes for this column.
 	backing := addColumn(b, spec, t)
 	if idx != nil {
@@ -458,6 +474,7 @@ type addColumnSpec struct {
 	compute          *scpb.ColumnComputeExpression
 	transientCompute *scpb.ColumnComputeExpression
 	comment          *scpb.ColumnComment
+	generatedAsID    *scpb.ColumnGeneratedAsIdentity
 	unique           bool
 	notNull          bool
 }
@@ -494,6 +511,9 @@ func addColumn(b BuildCtx, spec addColumnSpec, n tree.NodeFormatter) (backing *s
 		if spec.comment != nil {
 			b.Add(spec.comment)
 		}
+		if spec.generatedAsID != nil {
+			b.Add(spec.generatedAsID)
+		}
 		// Don't need to modify primary indexes for virtual columns.
 		if spec.colType.IsVirtual {
 			return getLatestPrimaryIndex(b, spec.tbl.TableID)