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

Enable Alibaba Cloud provider instances with expected ProviderID #2777

Merged
merged 1 commit into from Oct 20, 2021

Conversation

kwoodson
Copy link
Contributor

- What I did
This code enables the Alibaba Cloud provider platform on OpenShift. The Alibaba provider requires that the instances have a specific format in which the documentation states should be in "${REGION}.${INSTANCE_ID}".

This patch follows the pattern for other providers which sets an environment variable named KUBELET_NODE_NAME in /etc/systemd/system/kubelet.service.d/. The environment variable is set through creating a service that upon start will query the metadata service for the region and the instanceid.

This pull request requires #2772.

- How to verify it
I was able to verify that this code works by deploying the following work:

- Description for the changelog
Enabling support for Alibaba Cloud provider node names

@sinnykumari
Copy link
Contributor

There is already an open PR for updating OpenShift libs #2772. Can you please consolidate changes into one PR and close one of them?

@kwoodson
Copy link
Contributor Author

@sinnykumari Thanks for looking. There is some discussion going on for #2772 currently regarding the PodSecurity: True.

I feel better about leaving them separate as the 2772 updates the API, library-go, and client-go which is independent of these changes. If you would prefer to join them together I can do so.

@sinnykumari
Copy link
Contributor

@kwoodson
Copy link
Contributor Author

@sinnykumari This is built on top of 2772 so it includes the library updates.

Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

@bd233
Copy link

bd233 commented Sep 28, 2021

@kwoodson
Hello Kenny. Regarding the issue of 'ProviderID', I would like to know whether I need to make some changes in the Installer repo in addition to the modification of this library?

@kikisdeliveryservice kikisdeliveryservice self-assigned this Sep 28, 2021
@kikisdeliveryservice
Copy link
Contributor

just adding a hold until #2772 merges

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021
@sinnykumari
Copy link
Contributor

Removing hold now since #2772 has got merged
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021
@kikisdeliveryservice
Copy link
Contributor

@rvanderp3 @fabianofranz PTAL

Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

I'm a little (a lot) rusty on ratcheting in a new cloud provider, but just to understand this cloud not using cloud.conf flag?

func cloudProvider(cfg RenderConfig) (interface{}, error) {

(see 335-396)

I'd expect that this Test should also be updated:

func TestCloudProvider(t *testing.T) {

@kwoodson kwoodson force-pushed the ali_ignite branch 2 times, most recently from a8c75fb to 575de61 Compare October 1, 2021 13:29
@kwoodson
Copy link
Contributor Author

kwoodson commented Oct 1, 2021

I'm a little (a lot) rusty on ratcheting in a new cloud provider, but just to understand this cloud not using cloud.conf flag?

func cloudProvider(cfg RenderConfig) (interface{}, error) {

(see 335-396)
I'd expect that this Test should also be updated:

func TestCloudProvider(t *testing.T) {

Thanks @kikisdeliveryservice. I've been out for a couple of days.

Regarding the cloudProvider function this is handled by the isCloudProviderExternal which came in through the library-go update in #2772. The out-of-tree provider should return external as its value and is set in the kubelet.service after the mco runs.

I rebased this PR since 2772 was merged. This removed the vendored files and reduced this down to the Alibaba specific augmentation.

I have also added a test for Alibaba that should return external. Thanks for this suggestion I had missed it!

Copy link
Contributor

@sinnykumari sinnykumari left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2021
@kikisdeliveryservice
Copy link
Contributor

waiting for final review, since you're new owners of this can you PTAL @rvanderp3 @fabianofranz?

@rvanderp3
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 19, 2021
@kikisdeliveryservice kikisdeliveryservice removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2021
@kwoodson
Copy link
Contributor Author

@kikisdeliveryservice I had a team member take a look at this. Apologies for the delay we have been in training the previous week. Did you have anything else required for this PR?

@kikisdeliveryservice
Copy link
Contributor

Planning to give it a final review today/tomorrow :)

@kikisdeliveryservice
Copy link
Contributor

/retest-required

Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

This seems to be in good shape, thanks for your patience and hard work. Let's merge. =)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kikisdeliveryservice, kwoodson, rvanderp3, sinnykumari

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:
  • OWNERS [kikisdeliveryservice,sinnykumari]

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2021

@kwoodson: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-e2e-aws 887173f link false /test okd-e2e-aws
ci/prow/e2e-aws-workers-rhel7 887173f link false /test e2e-aws-workers-rhel7
ci/prow/e2e-aws-workers-rhel8 887173f link false /test e2e-aws-workers-rhel8
ci/prow/e2e-metal-ipi 887173f link false /test e2e-metal-ipi
ci/prow/e2e-aws-disruptive 887173f link false /test e2e-aws-disruptive

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 280d029 into openshift:master Oct 20, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants