[cluster-autoscaler] feat: Add equinix metal environment variables and also support older environment variables #6085

aayushrangwala · 2023-09-04T17:20:10Z

What type of PR is this?

/kind cleanup
/kind deprecation

What this PR does / why we need it:

As part of an effort to deprecate Packet cloud provider and support equinix metal names, after acquisition. This PR will support a new expected env vars but also support older env vars as usual for backward compatibility.

This is a sequential PR depends on #6084

Which issue(s) this PR fixes:

Fixes #4286

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The packet provider and its configuration parameters are now deprecated in favor of equinixmetal

- The cluster-autoscaler --cloud-provider flag should now be set to equinixmetal.  For backward compatibility, "--cloud-provider=packet" continues to work
- "METAL_AUTH_TOKEN" replaces "PACKET_AUTH_TOKEN". For backward compatibility, the latter still works.
- "EQUINIX_METAL_MANAGER" replaces "PACKET_MANAGER". For backward compatibility, the latter still works.
- Each node managed by cloud-provider "equinixmetal" will be labeled with the "METAL_CONTROLLER_NODE_IDENTIFIER_LABEL" defined label. For backward compatibility, "PACKET_CONTROLLER_NODE_IDENTIFIER_LABEL" still works.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go

x13n · 2023-10-10T09:14:06Z

This is a breaking change - anyone previously using --cloud-provider=packet will now get crashing autoscaler. Please update the release note to reflect that.

Also - can anyone from the existing packet/OWNERS LGTM the changes? I can then approve for everything that happens outside of that directory.

/assign

aayushrangwala · 2023-10-13T13:23:04Z

This is a breaking change - anyone previously using --cloud-provider=packet will now get crashing autoscaler. Please update the release note to reflect that.

Also - can anyone from the existing packet/OWNERS LGTM the changes? I can then approve for everything that happens outside of that directory.

/assign

@x13n This will not be a breaking change. The fix added in this PR supports backward compatibility with --cloud-provider=packet or --cloud-provider=equinix both
cc @displague

displague · 2023-10-13T15:49:06Z

cluster-autoscaler/cloudprovider/packet/packet_cloud_provider.go

@@ -128,51 +136,51 @@ func (pcp *packetCloudProvider) NodeGroupForNode(node *apiv1.Node) (cloudprovide
 }

 // HasInstance returns whether a given node has a corresponding instance in this cloud provider
-func (pcp *packetCloudProvider) HasInstance(node *apiv1.Node) (bool, error) {
+func (pcp *equinixMetalCloudProvider) HasInstance(node *apiv1.Node) (bool, error) {


I'm curious what we are missing out on by not implemented these methods where it seems we could easily offer a meaningful lookup:

HasInstance

GetAvailableMachineTypes

Also for these methods that seem more important than lookups:

Refresh

Cleanup

(and any others that are "NotImplemented" / "not implemented")

displague · 2023-10-13T15:52:39Z

@aayushrangwala could you attach some easy to follow repro steps, perhaps a video of the walkthrough. I'd like to see that:

old Packet based install guides still works
new Equinix Metal instructions work
what it looks like to update/migrate an existing install

x13n

Thanks, the changes outside of packet cloudprovider look good to me. Will approve them after the PR is LGTMed.

cluster-autoscaler/cloudprovider/builder/builder_all.go

cluster-autoscaler/cloudprovider/builder/builder_packet.go

aayushrangwala · 2023-10-19T18:10:06Z

@aayushrangwala could you attach some easy to follow repro steps, perhaps a video of the walkthrough. I'd like to see that:

old Packet based install guides still works

new Equinix Metal instructions work

what it looks like to update/migrate an existing install

7d989c3f-1863-4098-8040-cfa8ce43d780.mp4

cluster-autoscaler/cloudprovider/packet/packet_node_group_test.go

displague · 2023-10-20T21:29:27Z

@x13n The "Tests / test-and-verify (pull_request)" job is failing over what seems to be package problems. This PR doesn't seem to change any go.mod, nor imports, nor introduce new packages that are not imported, so not sure how this would be related.

@aayushrangwala Rebase?

…ility Signed-off-by: Ayush Rangwala <[email protected]>

… usual

Signed-off-by: Ayush Rangwala <[email protected]>

aayushrangwala · 2023-10-23T12:34:51Z

@x13n The "Tests / test-and-verify (pull_request)" job is failing over what seems to be package problems. This PR doesn't seem to change any go.mod, nor imports, nor introduce new packages that are not imported, so not sure how this would be related.

@aayushrangwala Rebase?

Yes tried rebasing, still failing. Trying to debug whats the issue

Signed-off-by: Ayush Rangwala <[email protected]>

displague · 2023-10-25T17:00:55Z

/lgtm

displague · 2023-10-25T17:03:46Z

@x13n

Thanks, the changes outside of packet cloudprovider look good to me. Will approve them after the PR is LGTMed.
...

From #6084 (applies here too?)

Please update the release note to mention that you're introducing a new cloud provider and deprecating existing 'packet' in favor of the new one.

Is there a particular place where @aayushrangwala should do that?

x13n · 2023-10-26T07:49:46Z

Yes, there's a

Does this PR introduce a user-facing change?

question in the PR template, please update the first comment by adding an answer there. We're collecting these when releasing a new version.

aayushrangwala · 2023-10-26T12:59:25Z

Yes, there's a

Does this PR introduce a user-facing change?

question in the PR template, please update the first comment by adding an answer there. We're collecting these when releasing a new version.

@x13n Updated the description to have user-facing change comments

x13n · 2023-10-26T15:00:20Z

Thanks!

/approve

k8s-ci-robot · 2023-10-26T15:00:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aayushrangwala, x13n

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster-autoscaler/OWNERS~~ [x13n]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from BigDarkClown and cprivitere September 4, 2023 17:20

aayushrangwala changed the title ~~Rename metal env~~ Add metal environment variables and also support older environment variables Sep 4, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2023

displague reviewed Sep 15, 2023

View reviewed changes

cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go Show resolved Hide resolved

aayushrangwala force-pushed the rename-metal-env branch from d4b42d1 to 8f5d591 Compare September 15, 2023 17:07

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2023

aayushrangwala force-pushed the rename-metal-env branch from 8f5d591 to 272a674 Compare September 18, 2023 15:50

aayushrangwala changed the title ~~Add metal environment variables and also support older environment variables~~ feat!: Add metal environment variables and also support older environment variables Sep 21, 2023

aayushrangwala changed the title ~~feat!: Add metal environment variables and also support older environment variables~~ feat: Add metal environment variables and also support older environment variables Sep 21, 2023

k8s-ci-robot assigned x13n Oct 10, 2023

aayushrangwala force-pushed the rename-metal-env branch from 272a674 to fd4f265 Compare October 12, 2023 16:23

displague reviewed Oct 13, 2023

View reviewed changes

x13n reviewed Oct 16, 2023

View reviewed changes

cluster-autoscaler/cloudprovider/builder/builder_all.go Outdated Show resolved Hide resolved

cluster-autoscaler/cloudprovider/builder/builder_packet.go Outdated Show resolved Hide resolved

displague reviewed Oct 20, 2023

View reviewed changes

cluster-autoscaler/cloudprovider/packet/packet_node_group_test.go Outdated Show resolved Hide resolved

aayushrangwala added 5 commits October 23, 2023 17:33

Rename comments, logs, structs, and vars from packet to equinix metal

d88664a

Rename types

4c75ced

fix: provider name to be used in builder to provide backward compatib…

1ec0667

…ility Signed-off-by: Ayush Rangwala <[email protected]>

Rename comments, logs, structs, and vars from packet to equinix metal

e4a4747

Created a new env var for metal to replace/support packet env vars as…

b00ec61

… usual

Support backward compatibility for PACKET_MANAGER env var

0d84ab6

Signed-off-by: Ayush Rangwala <[email protected]>

aayushrangwala force-pushed the rename-metal-env branch from fd4f265 to 950181f Compare October 23, 2023 12:09

aayushrangwala force-pushed the rename-metal-env branch from 950181f to 3a95773 Compare October 23, 2023 12:52

aayushrangwala changed the title ~~feat: Add metal environment variables and also support older environment variables~~ [cluster-autoscaler] feat: Add metal environment variables and also support older environment variables Oct 23, 2023

fix: refactor cloud provider names

1e4cb18

Signed-off-by: Ayush Rangwala <[email protected]>

aayushrangwala force-pushed the rename-metal-env branch from 3a95773 to 1e4cb18 Compare October 23, 2023 13:20

aayushrangwala changed the title ~~[cluster-autoscaler] feat: Add metal environment variables and also support older environment variables~~ [cluster-autoscaler] feat: Add equinix metal environment variables and also support older environment variables Oct 23, 2023

k8s-ci-robot assigned displague Oct 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023

aayushrangwala requested a review from x13n October 26, 2023 13:54

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023

k8s-ci-robot merged commit f872258 into kubernetes:master Oct 26, 2023
4 checks passed

aayushrangwala mentioned this pull request Oct 27, 2023

[WIP] feat: Rename comments, logs, structs, and vars from packet to equinix metal #6084

Closed

aayushrangwala mentioned this pull request Nov 10, 2023

[cluster-autoscaler] chore: Rename Cluster Autoscaler Provider package from Packet to EquinixMetal #6271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cluster-autoscaler] feat: Add equinix metal environment variables and also support older environment variables #6085

[cluster-autoscaler] feat: Add equinix metal environment variables and also support older environment variables #6085

aayushrangwala commented Sep 4, 2023 •

edited

Loading

x13n commented Oct 10, 2023

aayushrangwala commented Oct 13, 2023

displague Oct 13, 2023

displague Oct 13, 2023 •

edited

Loading

displague commented Oct 13, 2023

x13n left a comment

aayushrangwala commented Oct 19, 2023 •

edited

Loading

displague commented Oct 20, 2023 •

edited

Loading

aayushrangwala commented Oct 23, 2023

displague commented Oct 25, 2023

displague commented Oct 25, 2023 •

edited

Loading

x13n commented Oct 26, 2023

aayushrangwala commented Oct 26, 2023

x13n commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

[cluster-autoscaler] feat: Add equinix metal environment variables and also support older environment variables #6085

[cluster-autoscaler] feat: Add equinix metal environment variables and also support older environment variables #6085

Conversation

aayushrangwala commented Sep 4, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

x13n commented Oct 10, 2023

aayushrangwala commented Oct 13, 2023

displague Oct 13, 2023

Choose a reason for hiding this comment

displague Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

displague commented Oct 13, 2023

x13n left a comment

Choose a reason for hiding this comment

aayushrangwala commented Oct 19, 2023 • edited Loading

displague commented Oct 20, 2023 • edited Loading

aayushrangwala commented Oct 23, 2023

displague commented Oct 25, 2023

displague commented Oct 25, 2023 • edited Loading

x13n commented Oct 26, 2023

aayushrangwala commented Oct 26, 2023

x13n commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

aayushrangwala commented Sep 4, 2023 •

edited

Loading

displague Oct 13, 2023 •

edited

Loading

aayushrangwala commented Oct 19, 2023 •

edited

Loading

displague commented Oct 20, 2023 •

edited

Loading

displague commented Oct 25, 2023 •

edited

Loading