feat: ListNodeImageVersions + shared image gallery support #526

Bryce-Soghigian · 2024-10-16T03:12:57Z

Fixes #

Description
This pr introduces

USE_SIG option that is used to control if we should or should not use shared image galleries
Shared Image Gallery ID Resolution
New ListNodeImageVersions logic for getting the latest approved agentbaker node image version rather than using galleries as a source of truth

Open Qs:
Are we ok with all Nodes with Community Image gallery node images being drifted over to use SIG in managed karpenter with this change?

How was this change tested?

manual testing on MSFT Tenant SIG Galleries for Ubuntu and Azure Linux
make az-all
make test

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

Release Note

coveralls · 2024-10-16T03:16:55Z

Pull Request Test Coverage Report for Build 11371322239

Details

470 of 547 (85.92%) changed or added relevant lines in 13 files are covered.
19 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.2%) to 97.675%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/providers/imagefamily/types.go	8	12	66.67%
pkg/providers/instance/azure_client.go	1	7	14.29%
pkg/providers/imagefamily/image.go	50	79	63.29%
pkg/providers/imagefamily/nodeimageversionsclient.go	0	38	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/providers/instance/azure_client.go	1	15.0%
pkg/providers/imagefamily/image.go	1	69.81%
pkg/cache/unavailableofferings.go	2	95.45%
pkg/fake/communityimageversionsapi.go	15	46.67%

Totals
Change from base Build 11299903000:	-0.2%
Covered Lines:	36716
Relevant Lines:	37590

💛 - Coveralls

Bryce-Soghigian · 2024-10-16T03:23:49Z

pkg/fake/nodeimageversionsapi.go

+func (n NodeImageVersionsAPI) List(_ context.Context, _, _ string) (imagefamily.NodeImageVersionsResponse, error) {
+	return imagefamily.NodeImageVersionsResponse{
+		Values: []imagefamily.NodeImageVersion{
+			{


TODO: Codegen this data

Bryce-Soghigian · 2024-10-16T06:40:34Z

pkg/cloudprovider/drift.go


-	expectedImageID, err := c.imageProvider.GetImageID(ctx, communityImageName, publicGalleryURL, nodeClass.Spec.GetImageVersion())
+	expectedImageID, err := c.imageProvider.GetLatestImageID(ctx, imageStub, nodeClass.Spec.GetImageVersion())


note this will trigger drift when we switch it on going from community to sig

Bryce-Soghigian · 2024-10-16T06:41:09Z

pkg/operator/options/options.go

@@ -90,6 +94,8 @@ func (o *Options) AddFlags(fs *coreoptions.FlagSet) {
 	fs.StringVar(&o.SubnetID, "vnet-subnet-id", env.WithDefaultString("VNET_SUBNET_ID", ""), "The default subnet ID to use for new nodes. This must be a valid ARM resource ID for subnet that does not overlap with the service CIDR or the pod CIDR")
 	fs.Var(newNodeIdentitiesValue(env.WithDefaultString("NODE_IDENTITIES", ""), &o.NodeIdentities), "node-identities", "User assigned identities for nodes.")
 	fs.StringVar(&o.NodeResourceGroup, "node-resource-group", env.WithDefaultString("AZURE_NODE_RESOURCE_GROUP", ""), "[REQUIRED] the resource group created and managed by AKS where the nodes live")
+	fs.BoolVar(&o.ManagedKarpenter, "managed-karpenter", env.WithDefaultBool("MANAGED_KARPENTER", false), "Whether Karpenter is managed by AKS or not.")


Suggested change

fs.BoolVar(&o.ManagedKarpenter, "managed-karpenter", env.WithDefaultBool("MANAGED_KARPENTER", false), "Whether Karpenter is managed by AKS or not.")

fs.BoolVar(&o.ManagedKarpenter, "managed-karpenter", env.WithDefaultBool("MANAGED_KARPENTER", false), "[MANAGED-FLAG] Whether Karpenter is managed by AKS or not.")

Bryce-Soghigian · 2024-10-16T06:41:36Z

pkg/operator/options/options.go

@@ -90,6 +94,8 @@ func (o *Options) AddFlags(fs *coreoptions.FlagSet) {
 	fs.StringVar(&o.SubnetID, "vnet-subnet-id", env.WithDefaultString("VNET_SUBNET_ID", ""), "The default subnet ID to use for new nodes. This must be a valid ARM resource ID for subnet that does not overlap with the service CIDR or the pod CIDR")
 	fs.Var(newNodeIdentitiesValue(env.WithDefaultString("NODE_IDENTITIES", ""), &o.NodeIdentities), "node-identities", "User assigned identities for nodes.")
 	fs.StringVar(&o.NodeResourceGroup, "node-resource-group", env.WithDefaultString("AZURE_NODE_RESOURCE_GROUP", ""), "[REQUIRED] the resource group created and managed by AKS where the nodes live")
+	fs.BoolVar(&o.ManagedKarpenter, "managed-karpenter", env.WithDefaultBool("MANAGED_KARPENTER", false), "Whether Karpenter is managed by AKS or not.")
+	fs.StringVar(&o.SIGSubscriptionID, "sig-subscription-id", env.WithDefaultString("SIG_SUBSCRIPTION_ID", ""), "The subscription ID of the shared image gallery.")


Suggested change

fs.StringVar(&o.SIGSubscriptionID, "sig-subscription-id", env.WithDefaultString("SIG_SUBSCRIPTION_ID", ""), "The subscription ID of the shared image gallery.")

fs.StringVar(&o.SIGSubscriptionID, "sig-subscription-id", env.WithDefaultString("SIG_SUBSCRIPTION_ID", ""), "[MANAGED-FLAG] The subscription ID of the shared image gallery we want to point to")

rakechill · 2024-10-16T18:12:56Z

pkg/operator/options/options.go

@@ -74,6 +74,10 @@ type Options struct {
 	SubnetID string // => VnetSubnetID to use (for nodes in Azure CNI Overlay and Azure CNI + pod subnet; for for nodes and pods in Azure CNI), unless overridden via AKSNodeClass
 	setFlags map[string]bool

+	ManagedKarpenter bool // => ManagedKarpenter is true if Karpenter is managed by AKS, false if it is a self-hosted karpenter installation


remind me: is there any way to prevent self-hosted users from setting this?

no, presumably they good set it if they wanted to, but after we put Agentbaker bootstrapping behind this flag bootstrapping won't work for them.

I guess we could also determine if we are using managed based on the kubeclients or build path(main_ccp.go vs main.go) but i thought we wanted to remove those and consolidate them eventually.

Maybe this should be an enum?

Mode:

Managed

SelfHosted

With default SelfHosted?

@matthchr, feel like I remember best practice of using enums instead of bools?

charliedmcb · 2024-10-17T21:39:08Z

pkg/providers/imagefamily/image.go

 }

 const (
 	kubernetesVersionCacheKey = "kubernetesVersion"

+	// if imageVersion is equal to an "", then we will get the latest version
+	autoUpgradeMode = ""


Since we are looking to remove imageVersion I think leaving this out for now would be best.

Oh, missed this is a const. Makes sense for clarity. However, should still be getting removed.

tallaxes

Pease resolve merge conflicts (which should include removal of AKSNodeClass imageVersion)

tallaxes · 2024-11-12T23:06:35Z

pkg/providers/imagefamily/image.go

+// /subscriptions/{subscriptionID}/resourceGroups/{resourceGroup}/providers/Microsoft.Compute/galleries/{galleryName}/images/{imageDefinition}/versions/{imageVersion}.
+// If imageVersion is an empty string, it fetches the latest version of the image and caches it for imageExpirationInterval (3 days).
+func (p *Provider) getImageIDSIG(ctx context.Context, imgStub DefaultImageOutput, imageVersion string) (string, error) {
+	key := fmt.Sprintf(sharedImageGalleryImageIDFormat, options.FromContext(ctx).SIGSubscriptionID, imgStub.GalleryResourceGroup, imgStub.GalleryName, imgStub.ImageDefinition, imageVersion)


Why does subscription ID need to be part of the key?

We could reconstruct the full id without the cache including subscription id. Especially since sub is a constant. Good catch.

tallaxes · 2024-11-12T23:07:06Z

pkg/providers/imagefamily/image.go

+	if imageID, ok := p.imageCache.Get(key); ok {
+		return imageID.(string), nil
+	}
+	if imageVersion == autoUpgradeMode {


Looking forward to simplified code w/o imageVersion

tallaxes · 2024-11-12T23:08:04Z

pkg/providers/imagefamily/image.go

+				imageID, imageExpirationInterval)
+		}
+		// return the latest version of the image from the cache after we have caached all of the imageDefinitions
+		if imageID, ok := p.imageCache.Get(key); ok {


What guarantees this key will be available?

We iterate through all the image definitions + image versions from ListNodeImageVersions call before this section and set them in the cache.

for _, version := range versions.Values { imageID := fmt.Sprintf(sharedImageGalleryImageIDFormat, options.FromContext(ctx).SIGSubscriptionID, imgStub.GalleryResourceGroup, imgStub.GalleryName, version.SKU, version.Version) cacheKey := fmt.Sprintf(sharedImageGalleryImageIDFormat, options.FromContext(ctx).SIGSubscriptionID, imgStub.GalleryResourceGroup, imgStub.GalleryName, version.SKU, autoUpgradeMode) p.imageCache.Set( cacheKey, imageID, imageExpirationInterval) }

pkg/providers/imagefamily/image.go

tallaxes · 2024-11-12T23:09:57Z

pkg/providers/instance/azure_client.go

@@ -137,11 +140,13 @@ func NewAZClient(ctx context.Context, cfg *auth.Config, env *azure.Environment)
 	}
 	klog.V(5).Infof("Created azure resource graph client %v, using a token credential", azureResourceGraphClient)

-	imageVersionsClient, err := armcomputev5.NewCommunityGalleryImageVersionsClient(cfg.SubscriptionID, cred, opts)
+	communityImageVersionsClient, err := armcomputev5.NewCommunityGalleryImageVersionsClient(cfg.SubscriptionID, cred, opts)


This client may not be needed, based on SIG flag, right? Would be good to only instantiate clients that are needed.

tallaxes · 2024-11-12T23:13:01Z

pkg/operator/options/options.go

@@ -90,6 +94,8 @@ func (o *Options) AddFlags(fs *coreoptions.FlagSet) {
 	fs.StringVar(&o.SubnetID, "vnet-subnet-id", env.WithDefaultString("VNET_SUBNET_ID", ""), "The default subnet ID to use for new nodes. This must be a valid ARM resource ID for subnet that does not overlap with the service CIDR or the pod CIDR")
 	fs.Var(newNodeIdentitiesValue(env.WithDefaultString("NODE_IDENTITIES", ""), &o.NodeIdentities), "node-identities", "User assigned identities for nodes.")
 	fs.StringVar(&o.NodeResourceGroup, "node-resource-group", env.WithDefaultString("AZURE_NODE_RESOURCE_GROUP", ""), "[REQUIRED] the resource group created and managed by AKS where the nodes live")
+	fs.BoolVar(&o.ManagedKarpenter, "use-sig", env.WithDefaultBool("USE_SIG", false), "If set to true karpenter will use the AKS managed shared image galleries and the node image versions api. If set to false karpenter will use community image galleries. Only a subset of image features will be available in the community image galleries and this flag is only for the managed node provisioning addon.")


Can we rename the internal variable?

tallaxes · 2024-11-12T23:14:36Z

pkg/providers/imagefamily/image.go

+// GetLatestImageID returns the latest image ID for the given image version and image specification
+func (p *Provider) GetLatestImageID(ctx context.Context, defaultImage DefaultImageOutput, imageVersion string) (string, error) {
+	// Managed Karpenter will use the AKS Managed Shared Image Galleries
+	if options.FromContext(ctx).ManagedKarpenter {


Can one cache here, instead of later?

…ng out unsupported galleries

Bryce-Soghigian · 2024-11-15T09:51:38Z

<TODO: Move from 04-02 preview to 09-02 preview>

tallaxes

Failing CI

Bryce-Soghigian commented Oct 16, 2024

View reviewed changes

Bryce-Soghigian marked this pull request as ready for review October 16, 2024 06:35

Bryce-Soghigian commented Oct 16, 2024

View reviewed changes

rakechill reviewed Oct 16, 2024

View reviewed changes

Bryce-Soghigian self-assigned this Oct 17, 2024

Bryce-Soghigian mentioned this pull request Oct 17, 2024

BREAKING CHANGE(aksnodeclass): removing imageVersion from CRD #531

Merged

3 tasks

charliedmcb reviewed Oct 17, 2024

View reviewed changes

tallaxes reviewed Nov 12, 2024

View reviewed changes

Bryce-Soghigian added 19 commits November 14, 2024 23:53

feat: support shared image galleries inside of karpenter

1cb133c

chore: populating image stubs for shared image galleries

db34e35

fix: progress

ac5e943

fix: PopulateResourceStub accessing the wrong index

78fc1ec

test: properly testing ListNodeImageVersions

24ec40f

refactor: rename symbol for SIG Subscription id

c79ecfd

test: conditional use of sig dependent on the managed karpenter flag

f19c719

refactor: removing panics used in testing

107b41f

test: adding RBAC and helm values to the template for SIG Gallery logic

9c179cc

fix: bug in azure linux sig image resolution

3f7f761

chore: update cleanupenv to handle inflate too ratehr than just job pods

0efac0d

test: fix randomized test order flake

637e30e

ci: shadow declaration

0d7a98a

refactor: comment wording

13b410d

test: validate all image ids are resolved correctly

05e4ac9

fix: adding filtering for duplicate sku + os combinations and filteri…

283484d

…ng out unsupported galleries

refactor: renaming var

227f7d6

refactor: rename the managedKarpenter reference to UseSIG

340f481

refactor: spelling

1901a83

fix: v1 migration for test

cc07eb7

Bryce-Soghigian force-pushed the bsoghigian/list-node-image-versions-poc branch from 321f082 to cc07eb7 Compare November 15, 2024 09:41

Bryce-Soghigian added 2 commits November 15, 2024 02:03

ci: lint

4b05f7f

fix: lint

29deb55

tallaxes requested changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ListNodeImageVersions + shared image gallery support #526

feat: ListNodeImageVersions + shared image gallery support #526

Bryce-Soghigian commented Oct 16, 2024 •

edited

Loading

coveralls commented Oct 16, 2024 •

edited

Loading

Bryce-Soghigian Oct 16, 2024

Bryce-Soghigian Oct 16, 2024

Bryce-Soghigian Oct 16, 2024

Bryce-Soghigian Oct 16, 2024

rakechill Oct 16, 2024

Bryce-Soghigian Oct 16, 2024

charliedmcb Oct 17, 2024 •

edited

Loading

charliedmcb Oct 17, 2024

charliedmcb Oct 17, 2024

tallaxes left a comment

tallaxes Nov 12, 2024

Bryce-Soghigian Nov 14, 2024

tallaxes Nov 12, 2024

tallaxes Nov 12, 2024

Bryce-Soghigian Nov 14, 2024 •

edited

Loading

tallaxes Nov 12, 2024

tallaxes Nov 12, 2024

Bryce-Soghigian Nov 14, 2024

tallaxes Nov 12, 2024

Bryce-Soghigian commented Nov 15, 2024

tallaxes left a comment


		expectedImageID, err := c.imageProvider.GetImageID(ctx, communityImageName, publicGalleryURL, nodeClass.Spec.GetImageVersion())
		expectedImageID, err := c.imageProvider.GetLatestImageID(ctx, imageStub, nodeClass.Spec.GetImageVersion())

	fs.BoolVar(&o.ManagedKarpenter, "managed-karpenter", env.WithDefaultBool("MANAGED_KARPENTER", false), "Whether Karpenter is managed by AKS or not.")
	fs.BoolVar(&o.ManagedKarpenter, "managed-karpenter", env.WithDefaultBool("MANAGED_KARPENTER", false), "[MANAGED-FLAG] Whether Karpenter is managed by AKS or not.")

	fs.StringVar(&o.SIGSubscriptionID, "sig-subscription-id", env.WithDefaultString("SIG_SUBSCRIPTION_ID", ""), "The subscription ID of the shared image gallery.")
	fs.StringVar(&o.SIGSubscriptionID, "sig-subscription-id", env.WithDefaultString("SIG_SUBSCRIPTION_ID", ""), "[MANAGED-FLAG] The subscription ID of the shared image gallery we want to point to")

feat: ListNodeImageVersions + shared image gallery support #526

Are you sure you want to change the base?

feat: ListNodeImageVersions + shared image gallery support #526

Conversation

Bryce-Soghigian commented Oct 16, 2024 • edited Loading

coveralls commented Oct 16, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11371322239

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliedmcb Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallaxes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bryce-Soghigian Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bryce-Soghigian commented Nov 15, 2024

tallaxes left a comment

Choose a reason for hiding this comment

Bryce-Soghigian commented Oct 16, 2024 •

edited

Loading

coveralls commented Oct 16, 2024 •

edited

Loading

charliedmcb Oct 17, 2024 •

edited

Loading

Bryce-Soghigian Nov 14, 2024 •

edited

Loading