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

[Digital Ocean] Promote to Beta #10312

Merged
merged 7 commits into from
Dec 4, 2020
Merged

Conversation

srikiz
Copy link
Contributor

@srikiz srikiz commented Nov 24, 2020

Hi,
I wanted thoughts on promoting Digital Ocean to Beta. Currently, a good amount of KOPS functionality is in place for DO, and I wanted to check with the KOPS maintainers if you are okay with this promotion.

Tested with both single cluster and multi-master HA setup
Tested kubernetes upgrade with kops update feature (ex: from v1.19.3 to v1.19.4)
Tested kops validate command on cluster creation
Tested creating multiple instance groups and managing & updating & deleting it.
Tested using the cluster with a DO load balancer when a new service with type: LoadBalancer is used.
Tested creating new PV with CSI drivers for managing DO volumes.

FYI - @timoreimann

@k8s-ci-robot
Copy link
Contributor

Hi @srikiz. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 24, 2020
@k8s-ci-robot k8s-ci-robot added area/documentation area/provider/digitalocean Issues or PRs related to digitalocean provider labels Nov 24, 2020
@fejta-bot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 24, 2020
@olemarkus
Copy link
Member

I guess a concern from my side is who will own this going forward. I think at least one person who contribute outside of only this provider should commit to also maintain this. Beta is fairly close to stable in our world.

@srikiz the stuff you have been doing here is pretty solid. I don't know if you are interested in contributing to kops as a whole?

Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

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

I would definitely like to see DO promoted to beta. I think its worth clarifying exactly what the criteria is for beta and stable cloud providers. Perhaps we discuss it at the next office hours?

I think it would be great to have automated e2e tests for DO. I believe cluster-api has conformance tests and presubmits running, so I would think much of the setup work is already taken care of if we can get permission to share the DO account with Cluster API. I'm happy to assist with setting up the prow jobs in test-infra once we're ready. I think just periodic tests (similar to GCP's) would be fine.

upup/pkg/fi/cloudup/apply_cluster.go Outdated Show resolved Hide resolved
@srikiz
Copy link
Contributor Author

srikiz commented Nov 25, 2020

I guess a concern from my side is who will own this going forward. I think at least one person who contribute outside of only this provider should commit to also maintain this. Beta is fairly close to stable in our world.

@srikiz the stuff you have been doing here is pretty solid. I don't know if you are interested in contributing to kops as a whole?

Thanks @olemarkus - I would like to contribute to kops as a whole, but currently can't commit to it. May be we can discuss more during our next KOPS meeting.

@timoreimann
Copy link
Contributor

timoreimann commented Nov 26, 2020

👋 Timo here from DigitalOcean. I've been supporting @srikiz over the last few months as far as support for DigitalOcean in kops is concerned.

We raised the topic on what it takes for a provider to be promoted to beta in a prior office hours meeting on 13. April 2020. Unfortunately, not too many points on that discussion were apparently noted down in the meeting notes. However, I did take notes myself back then and wrote down the following requirements as discussed during the office hours:

  • Cluster does not need to be destroyed anymore for rolling upgrade
  • Implement validation for DO executed during kops create / update
  • HA support

To my understanding, these TODOs are now completed. (@srikiz, correct me if I'm wrong here.)

I think we explicitly excluded test integration at the time. Not exactly sure about the reasons anymore, perhaps the level of support for / ease of adding tests was different back then?
That said, if test integration is expected today from all supported clouds, I'd be happy to talk about it. Let me know what you think, thanks!

@olemarkus
Copy link
Member

From my perspective, testing is super nice, but it is not a hard requirement. Timely fixes, keeping up with the development of kops overall etc is more important.

My fear is that DO goes beta, a lot of people starts using it, and then something breaks. Or that we need to update some deps, this breaks the provider, and there is no one around to fix it.

Now @srikiz almost immediately fixed a case of the latter issue earlier today, so I have full confidence that this is not a problem today. I just, perhaps, need a little bit more confidence that this won't be a problem next year.

If DO softly commits to support kops, even support progress DO to stable, then I will happily support DO going to beta in its current state.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2020
@rifelpet
Copy link
Member

I tracked down a recording of the office hours in which we discussed DO being promoted to beta: https://youtu.be/A6DXuR4Nu30?t=2584

Justin's points for reaching beta status:

  • API Stability
  • Going to GA won't require a cluster to be torn down and recreated

@srikiz mentioned an issue around using an internal load balancer for kubelets to reach the apiserver, possibly the useForInternalApi ClusterSpec field. Has that been addressed?

I agree that e2e testing needn't be a hard requirement for beta. I think if we fix the conflicts here and address any review comments then we can bring it up on Friday's office hours call for any final objections and plan to merge it then.

@srikiz
Copy link
Contributor Author

srikiz commented Nov 30, 2020

@rifelpet - I didn't use the useForInternalAPI behavior. As I understand, currently KOPS injects /etc/hosts file entries of all the master hosts and kubelet uses one of those host entries for communicating to api server. DO uses the same behavior in this case.

@rifelpet
Copy link
Member

rifelpet commented Dec 1, 2020

@srikiz ok that makes sense. Feel free to fix the conflicts here and we can discuss and hopefully merge this on Friday.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 4, 2020
@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

/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 Dec 4, 2020
@@ -1408,4 +1408,4 @@ sigs.k8s.io/yaml
# k8s.io/cli-runtime => k8s.io/cli-runtime v0.20.0-beta.2
# k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.20.0-beta.2
# k8s.io/code-generator => k8s.io/code-generator v0.20.0-beta.2
# github.com/gophercloud/gophercloud => github.com/gophercloud/gophercloud v0.11.0
Copy link
Member

Choose a reason for hiding this comment

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

i think this change should be reverted in order to make the verify-gomod job pass

@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

hm not sure why that job failure is still in the GH comment if the job itself passed...

/test pull-kops-verify-packages

@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

Everyone on the call today was happy to see DO get promoted to Beta so I'm going to go ahead and merge this 👍🏻 congrats!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet, srikiz

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 77b6da4 into kubernetes:master Dec 4, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Dec 4, 2020
@timoreimann
Copy link
Contributor

@rifelpet thanks a lot for your help on driving this one over the finishing line, really appreciate it. Excited to hopefully see more adoption of kops in DigitalOcean over time now that stability / maturity has improved.

@timoreimann
Copy link
Contributor

I have also filed #10386 to track the work needed to get proper e2e testing in place. Sri and I are happy to move that one forward as well.

k8s-ci-robot pushed a commit that referenced this pull request Dec 12, 2020
* Move DO to beta

* Update vendor files

* Update documentation

* Update vendor modules

* Remove AlphaAllowDO flag

* Fix vendor modules

* Revert vendor modules txt file

* Revert modules.txt changes to keep it in sync with release-1.19
@srikiz srikiz deleted the DO-Move-to-Beta branch October 25, 2021 17:38
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. area/documentation area/provider/digitalocean Issues or PRs related to digitalocean provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants