Skip to content

zones: initial implementation of Zones interface #143

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

zones: initial implementation of Zones interface #143

wants to merge 1 commit into from

Conversation

nicolehanjing
Copy link
Member

@nicolehanjing nicolehanjing commented Oct 12, 2020 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This is a first pass at implementing Zones.
Verified after node registration, the node has labels with zone/region information:

labels:
    beta.kubernetes.io/arch: amd64
    beta.kubernetes.io/instance-type: t3.large
    beta.kubernetes.io/os: linux
    failure-domain.beta.kubernetes.io/region: us-west-2
    failure-domain.beta.kubernetes.io/zone: us-west-2a
    kubernetes.io/arch: amd64
    kubernetes.io/hostname: ip-10-0-0-162.us-west-2.compute.internal
    kubernetes.io/os: linux
    node.kubernetes.io/instance-type: t3.large
    topology.kubernetes.io/region: us-west-2
    topology.kubernetes.io/zone: us-west-2a

Which issue(s) this PR fixes:

Part of #125

Special notes for your reviewer:
TODO:
We should add zones / region support via InstancesV2 since kubernetes/kubernetes#93569 was merged in v1.20, where zone/region is just added to InstanceMetadata and implemented as part of InstancesV2
Will update zones after v1.20

Does this PR introduce a user-facing change?:

Add the initial implementation of Zones for the v2 provider.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 12, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @nicolehanjing. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nicolehanjing
To complete the pull request process, please assign andrewsykim after the PR has been reviewed.
You can assign the PR to them by writing /assign @andrewsykim in a comment when ready.

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

Needs approval from an approver in each of these files:

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
Copy link
Contributor

@nicolehanjing: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nicolehanjing
Copy link
Member Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 13, 2020
@nicolehanjing nicolehanjing force-pushed the nicoleh-zones branch 3 times, most recently from bd03fc1 to a22f644 Compare October 21, 2020 21:29
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 21, 2020
nicolehanjing
nicolehanjing commented Oct 26, 2020
View reviewed changes
pkg/providers/v2/instances.go
@@ -138,32 +139,79 @@ func (i *instances) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloud
// getInstance returns the instance if the instance with the given node info still exists.
// If false an error will be returned, the instance will be immediately deleted by the cloud controller manager.
func (i *instances) getInstance(ctx context.Context, node *v1.Node) (*ec2.Instance, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

broke this function into 2 functinos: getInstanceByProviderID & getInstanceByPrivateDNSName since GetZoneByProviderID and GetZoneByNodeName can direcly call the corresponding function to get instance info

nicolehanjing
nicolehanjing commented Oct 26, 2020
View reviewed changes
pkg/providers/v2/zones.go
)

// newZones returns an implementation of cloudprovider.Zones
// TODO:
Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewsykim added a TODO note here

@nicolehanjing
Copy link
Member Author

/close
will include zone/region info in InstanceMetadata in v1.20

@k8s-ci-robot
Copy link
Contributor

@nicolehanjing: Closed this PR.

In response to this:

/close
will include zone/region info in InstanceMetadata in v1.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb Awaiting requested review from justinsb

@M00nF1sh M00nF1sh Awaiting requested review from M00nF1sh

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicolehanjing @k8s-ci-robot