Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: CSE-based bootstrapping with bootstrapping client mode #527

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

comtalyst
Copy link
Collaborator

@comtalyst comtalyst commented Oct 16, 2024

Fixes #

Description

Using provision servers provide opportunities for us to delegate deterministic bootstrapping details away to keep our maintenance focus on autoscaling.
For the first mode introduced here—the bootstrapping client—will also switch back to CSE-based bootstrapping for general stability. The server is expected to provide custom data and CSE based on our inputs of ProvisionProfile. See swagger file for specifics.

This pathway will be supported by NAP only, for now.

How was this change tested?

  • NAP E2Es
  • Manual inspection of retrieved CSE values
  • K8s versions: 1.28.0, 1.29.9, 1.30.1, 1.31.1
  • ACRPull (on the above versions)

Does this change impact docs?

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

Release Note

NONE

},
}
}

// UserData returns the default userdata script for the image Family
func (u AzureLinux) UserData(kubeletConfig *corev1beta1.KubeletConfiguration, taints []v1.Taint, labels map[string]string, caBundle *string, _ *cloudprovider.InstanceType) bootstrap.Bootstrapper {
func (u AzureLinux) SelfContainedUserData(kubeletConfig *corev1beta1.KubeletConfiguration, taints []v1.Taint, labels map[string]string, caBundle *string, _ *cloudprovider.InstanceType) bootstrap.Bootstrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to rename this and change from the AWS Paradigm we should call this CustomData instead of UserData?

This ends up getting converted to custom data here https://github.com/Azure/karpenter-provider-azure/blob/main/pkg/providers/instance/instance.go#L336

The reason we called it user data was to align with AWS but if we are changing might as well go all the way.

Copy link
Collaborator Author

@comtalyst comtalyst Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to self-contained feature owners, right now that is actually CustomData (yes, I think we've been mislabeling it). But there could be a switch soon. But we could just rename it for now.

@@ -60,6 +62,7 @@ func (u Ubuntu2204) DefaultImages() []DefaultImageOutput {
scheduling.NewRequirement(v1.LabelArchStable, v1.NodeSelectorOpIn, corev1beta1.ArchitectureAmd64),
scheduling.NewRequirement(v1alpha2.LabelSKUHyperVGeneration, v1.NodeSelectorOpIn, v1alpha2.HyperVGenerationV1),
),
Distro: "aks-ubuntu-containerd-22.04",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these should be constants

Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of comments, still need to dig into core logic a bit more and think through the new launch template patterns and the new resolver patterns also I need to evaluate if we are passing in all the right KubeletConfiguration.

azConfig.KubeletIdentityClientID,
azConfig.NodeResourceGroup,
azConfig.Location,
vnetGUID,
options.FromContext(ctx).ProvisionMode,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need provision mode or would it be better to align on a NAP flag?

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be more than one provisioning mode (including within NAP)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#527 (comment) yeah I responded down here with similar arguement. We could have multiple provisioning modes, also we could have mulitple "types" of karpenter

self-hosted aks, managed aks, self-hosted azure, its arguable that even this boolean flag I am introducing in my PR would make more sense as a string with multiple options.

if p.KubeletConfig != nil && p.KubeletConfig.MaxPods != nil {
provisionProfile.MaxPods = p.KubeletConfig.MaxPods
} else {
provisionProfile.MaxPods = lo.ToPtr(int32(250)) // Delegatable defaulting?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to delegate defaulting until nap users can configure different limits for MaxPods.

If we default for azure cni without overlay to 30, without a way for customers to change that value one node will never be able to have more than 30 pods.

I suspect that will be a pain point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MaxPods in AKSNodeClass is in the v1 migration PR, so CNI-based defaulting (client- or server-side) should work.

provisionProfile.MaxPods = lo.ToPtr(int32(250)) // Delegatable defaulting?
}

if modeString, ok := p.Labels["kubernetes.azure.com/mode"]; ok && modeString == "system" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the labels like this into our api and the modeString into our constants?

template.AgentBakerCSE = cse
} else {
// render user data
userData, err := params.SelfContainedUserData.Script()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is self contained ever using userdata? why is it called this? Maybe im misunderstanding something?

AgentBakerNodeBootstrapping agentbakerbootstrap.Bootstrapper
ImageID string
StorageProfile string
IsWindows bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from here and resolve with helper function instead since we already know image family?

if resp.Payload == nil {
return "", "", fmt.Errorf("no payload in response")
}
if resp.Payload.Cse == nil || *resp.Payload.Cse == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: CSE should be cased consistently across the code

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, though some of this may be coming from Swagger etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this come un-golang namings from the auto-generated client.

pkg/operator/options/options_validation.go Outdated Show resolved Hide resolved
Comment on lines 120 to 122
if p.KubeletConfig != nil {
provisionProfile.CustomKubeletConfig = &models.CustomKubeletConfig{
// AllowedUnsafeSysctls: ..., // Unsupported as of now
CPUCfsQuota: p.KubeletConfig.CPUCFSQuota,
}
}

Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave some comments here on whats defaulted in NPS for KubeletConfig vs what is not? Its very confusing to understand what flags we are getting vs need to be passing in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required fields are marked in the swagger spec.

if p.KubeletConfig != nil {
provisionProfile.CustomKubeletConfig = &models.CustomKubeletConfig{
// AllowedUnsafeSysctls: ..., // Unsupported as of now
CPUCfsQuota: p.KubeletConfig.CPUCFSQuota,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan on adding official support for customers specifying kubelet configuration for GA?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, first cut of a subset (essentially CustomKubeletConfig w/o CPU/MemoryReserved and Seccomp) is in v1 migration PR; we should pick this up when merging the two. (The set is relatively well-aligned, but minor adaptation will be needed.)

Comment on lines 45 to 64
type ProvisionClientBootstrap struct {
ClusterName string
KubeletConfig *corev1beta1.KubeletConfiguration
Taints []core.Taint `hash:"set"`
StartupTaints []core.Taint `hash:"set"`
Labels map[string]string `hash:"set"`
SubnetID string
Arch string
SubscriptionID string
ClusterResourceGroup string
ResourceGroup string
KubeletClientTLSBootstrapToken string
KubernetesVersion string
ImageDistro string
IsWindows bool
InstanceType *cloudprovider.InstanceType
StorageProfile string
ImageFamily string
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these options are shared between self contained and agentbaker bootstrap. Can we share those values with struct embedding rather than copy pasting in both places?

Comment on lines 105 to 109
if p.Arch == "amd64" {
provisionProfile.Architecture = lo.ToPtr("x64")
} else {
provisionProfile.Architecture = lo.ToPtr(p.Arch)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be defaulted based on the instance type on the NPS side?

Comment on lines 139 to 152
if utils.IsNvidiaEnabledSKU(p.InstanceType.Name) {
if utils.UseGridDrivers(p.InstanceType.Name) {
provisionProfile.GpuProfile = &models.GPUProfile{
DriverType: lo.ToPtr(models.DriverType_GRID),
InstallGPUDriver: lo.ToPtr(true),
}
} else {
provisionProfile.GpuProfile = &models.GPUProfile{
DriverType: lo.ToPtr(models.DriverType_CUDA),
InstallGPUDriver: lo.ToPtr(true),
}
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, all of this logic will resolve to the same static values based on the sku, if we have the sku NPS can resolve these values.

Comment on lines 154 to 140
if p.IsWindows {
provisionProfile.OsType = lo.ToPtr(models.OSType_Windows)
} else {
provisionProfile.OsType = lo.ToPtr(models.OSType_Linux)
}
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems odd we need this at all since we pass in OS SKU. This alongside some of these other values could be easily inferred in a more minimal AgentpoolProfile payload

Comment on lines 160 to 146
provisionHelperValues := &models.ProvisionHelperValues{
SkuCPU: lo.ToPtr(p.InstanceType.Capacity.Cpu().AsApproximateFloat64()),
SkuMemory: lo.ToPtr(p.InstanceType.Capacity.Memory().AsApproximateFloat64()),
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again would be nice to infer these based on the instance type. All values that are not user input probably don't need to be in the provision profile payload in the future. We can resolve them inside of NPS

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an architectural gap that I am going to look into later. Basically, it is not as simple to be resolved as other values, for now.

Comment on lines 196 to 181
if resp.Payload.CustomData == nil || *resp.Payload.CustomData == "" {
return "", "", fmt.Errorf("no CustomData in response")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we get errors here would it make sense to fall back onto SelfContained bootstrap? Could we fall back on our bootstrapping logic if this fails?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, but I think the answer is No

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could look into general retriability (e.g., from network/reliability issues) later.
But I don't think we should mix provision modes, for consistency.

Comment on lines 217 to 202
nodeLabels["kubernetes.azure.com/role"] = "agent"
nodeLabels["kubernetes.azure.com/cluster"] = normalizeResourceGroupNameForLabel(nodeResourceGroup)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make the karpenter nodepool aware of these defaulted labels?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, though this would only be consequential if there are workloads targeting these, which is very unlikely. (More generally, it may be useful to develop a mechanism for letting Karpenter know Nodes will have certain labels - without having to specify them in NodePool.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I doubt role and cluster will be labels people use for scheduling, i was thinking a similar thing to have some providerLabels concept that allows core to know about injected values that are created in the code without requiring them on the nodepool.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI those are already there in self-contained.
The new one we will get from the provision server (and AgentBaker) will be the below.
The emptiness is intentional to prevent workload targeting them, for now.

agentpool=
kubernetes.azure.com/agentpool=

@@ -60,6 +62,7 @@ func (u AzureLinux) DefaultImages() []DefaultImageOutput {
scheduling.NewRequirement(v1.LabelArchStable, v1.NodeSelectorOpIn, corev1beta1.ArchitectureAmd64),
scheduling.NewRequirement(v1alpha2.LabelSKUHyperVGeneration, v1.NodeSelectorOpIn, v1alpha2.HyperVGenerationV1),
),
Distro: "aks-azurelinux-v2",
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these consts, or can we import them from agentbaker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will consider that along with potential delegation of this field to the provision server.

if p.provisionMode == options.ProvisionModeBootstrappingClient {
err = p.createCSExtension(ctx, resourceName, launchTemplate.AgentBakerCSE, launchTemplate.IsWindows)
if err != nil {
// This should fall back to cleanupAzureResources
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be logging the CSE Failure at least? Or creating k8s events for CSE Failure?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged from within createCSExtension

Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great for the first (and working) cut! I presume fixing CI & tests is coming, and further refactoring/cleanup can be expected later.

if p.KubeletConfig != nil && p.KubeletConfig.MaxPods != nil {
provisionProfile.MaxPods = p.KubeletConfig.MaxPods
} else {
provisionProfile.MaxPods = lo.ToPtr(int32(250)) // Delegatable defaulting?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MaxPods in AKSNodeClass is in the v1 migration PR, so CNI-based defaulting (client- or server-side) should work.

if p.KubeletConfig != nil {
provisionProfile.CustomKubeletConfig = &models.CustomKubeletConfig{
// AllowedUnsafeSysctls: ..., // Unsupported as of now
CPUCfsQuota: p.KubeletConfig.CPUCFSQuota,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, first cut of a subset (essentially CustomKubeletConfig w/o CPU/MemoryReserved and Seccomp) is in v1 migration PR; we should pick this up when merging the two. (The set is relatively well-aligned, but minor adaptation will be needed.)


provisionHelperValues := &models.ProvisionHelperValues{
SkuCPU: lo.ToPtr(p.InstanceType.Capacity.Cpu().AsApproximateFloat64()),
SkuMemory: lo.ToPtr(p.InstanceType.Capacity.Memory().AsApproximateFloat64()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure units align

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see the update, there is an issue I found and a workaround)

Comment on lines 169 to 152
transport := httptransport.New(options.FromContext(ctx).NodeBootstrappingServerURL, "/", []string{"http"})
client := client.New(transport, strfmt.Default)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming client reuse, scheme lookup etc. will come with refactoring ...

ImageID string
SubnetID string
Tags map[string]*string
AgentBakerCustomData string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to carry both? Or is this intended as UserData vs CustomData separation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from just that intention, I don't think we truly need to separate it.
But I would prefer to keep it separated this before we are more confident in that.

Tags map[string]*string
AgentBakerCustomData string
AgentBakerCSE string
IsWindows bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not feel like it belongs in the template ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to spend more time on this category later. Will require a more thorough understanding of the abstraction intentions.

@Bryce-Soghigian Bryce-Soghigian added the area/bootstrap Issues or PRs related to bootstrap label Oct 17, 2024
Comment on lines 35 to 38
func GetAKSGPUImageSHA(size string) string {
if useGridDrivers(size) {
if UseGridDrivers(size) {
return AKSGPUGridSHA
}
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these values have to come from us? if we use the ones from agentbaker and default them in NPS we can have consistency and won't have to bump the shas for cves separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not using SHAs. Just that UseGridDrivers(). But could still be a candidate for that consideration.

@coveralls
Copy link

coveralls commented Oct 18, 2024

Pull Request Test Coverage Report for Build 11428938152

Details

  • 178 of 1522 (11.7%) changed or added relevant lines in 33 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.4%) to 94.477%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/providers/imagefamily/resolver.go 30 32 93.75%
pkg/providers/imagefamily/image.go 4 7 57.14%
pkg/providers/launchtemplate/launchtemplate.go 18 26 69.23%
pkg/provisionclients/client/node_bootstrapping_client.go 26 42 61.9%
pkg/provisionclients/models/agent_pool_security_profile.go 0 18 0.0%
pkg/provisionclients/models/agent_pool_windows_profile.go 0 18 0.0%
pkg/provisionclients/models/artifact_streaming_profile.go 0 18 0.0%
pkg/provisionclients/models/azure_o_s_image_config.go 0 18 0.0%
pkg/provisionclients/models/custom_kubelet_config.go 0 18 0.0%
pkg/provisionclients/models/g_p_u_profile.go 0 18 0.0%
Totals Coverage Status
Change from base Build 11414974949: -3.4%
Covered Lines: 36450
Relevant Lines: 38581

💛 - Coveralls

@comtalyst comtalyst force-pushed the comtalyst/nap-cse-bootstrapping branch from 3e891a2 to 9c691b3 Compare October 18, 2024 23:31
@comtalyst comtalyst marked this pull request as ready for review October 19, 2024 00:22
pkg/operator/options/options.go Outdated Show resolved Hide resolved
@@ -96,7 +97,7 @@ 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.StringVar(&o.ProvisionMode, "provision-mode", env.WithDefaultString("PROVISION_MODE", ""), "[UNSUPPORTED] The provision mode for the cluster.")
fs.StringVar(&o.ProvisionMode, "provision-mode", env.WithDefaultString("PROVISION_MODE", "aksselfcontained"), "[UNSUPPORTED] The provision mode for the cluster.")
fs.StringVar(&o.NodeBootstrappingServerURL, "nodebootstrapping-server-url", env.WithDefaultString("NODEBOOTSTRAPPING_SERVER_URL", ""), "[UNSUPPORTED] The url for the node bootstrapping provider server.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fs.StringVar(&o.NodeBootstrappingServerURL, "nodebootstrapping-server-url", env.WithDefaultString("NODEBOOTSTRAPPING_SERVER_URL", ""), "[UNSUPPORTED] The url for the node bootstrapping provider server.")
fs.StringVar(&o.NodeBootstrappingServerURL, "nodebootstrapping-server-url", env.WithDefaultString("NODEBOOTSTRAPPING_SERVER_URL", ""), "[UNSUPPORTED] The url for the node bootstrapping provider server")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of our options have the full stop in their descriptions.
Is not having it a right way?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, full stop seems correct. Just wanted it to be consistent, and looked at surrounding ones and not the full set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the other ones missing a full stop to have it.

"github.com/Azure/karpenter-provider-azure/pkg/providers/instancetype"
)

func TestReverseVMMemoryOverhead(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to keep this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better than not having it?
In case someone change something and it matters. And it is not too burdening resource-wise IMO.

pkg/operator/options/options.go Outdated Show resolved Hide resolved
},
"podMaxPids": {
"type": "integer",
"format": "int32",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that actual kubelet podPidsLimit is int64. Something we likely need to fix on the AKS side, might as well type it correctly here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should change ours after to not misalign with AKS requirements...

@@ -358,3 +358,11 @@ az-helm-install-snapshot: az-configure-values ## Install Karpenter snapshot rele

az-rmcrds: ## Delete Karpenter CRDs
kubectl delete crd nodepools.karpenter.sh nodeclaims.karpenter.sh aksnodeclasses.karpenter.azure.com

az-swagger-generate-clients-raw:
cd pkg/provisionclients && swagger generate client -f swagger/*.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of thoughts:

  • You should be able to remove make targets / fold them into just "go generate" by creating e.g. provisionclients/generate.go with something like //go:generate swagger generate client ... etc.
  • Take a look at generate client options, consider maybe --keep-spec-order?
  • Could we maybe keep the spec in yaml instead of json?

@comtalyst comtalyst force-pushed the comtalyst/nap-cse-bootstrapping branch from 9c691b3 to ff4706a Compare October 19, 2024 08:25
@@ -28,4 +28,7 @@ const (
NetworkDataplaneAzure = "azure"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, most of these should be in options to not have knowledge coupling.
But for now, let's keep it in one place for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bootstrap Issues or PRs related to bootstrap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants