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

pkg/ansible/controller,pkg/helm/controller; Enable status subresource by default #787

Merged
merged 16 commits into from
Dec 11, 2018

Conversation

dymurray
Copy link
Contributor

@dymurray dymurray commented Nov 29, 2018

Description of the change:
Enable use of status subresource by default in operator-sdk.

Ansible:
Adds support for hybrid managed status between AO and user's Ansible. Get's latest version of resource to ensure that we are updating the latest version of the resource.

Motivation for the change:
In Kubernetes v1.11.0 the status subresource is enabled by default and so this PR enables the status subresource by default in operator-sdk.

Ansible:
Hybrid use case panics when user updates status from Ansible task.

Closes #809

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 29, 2018
@openshift-ci-robot
Copy link

Hi @dymurray. Thanks for your PR.

I'm waiting for a operator-framework 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.

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.

@shawn-hurley shawn-hurley added the language/ansible Issue is related to an Ansible operator project label Nov 29, 2018
@shawn-hurley
Copy link
Member

@dymurray Before we merge this we need to make sure that we add some migration steps to some sort of doc to describe what it looks like if you're using a CRD without sub-resources and what to do to fix it.

@shawn-hurley
Copy link
Member

/hold

this would require v1.12 of kubernetes by default. @fabianvf @dymurray we may need to rethink this, my apolgies I forgot about this caveat when discussing this.

Lets plan on talking at some point today in person to determine what to do.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2018
@dymurray
Copy link
Contributor Author

dymurray commented Dec 4, 2018

Just correcting Shawn's comment, this requires v1.11 of Kubernetes by default:
kubernetes/kubernetes#63598

@hasbro17
Copy link
Contributor

hasbro17 commented Dec 5, 2018

fixes: #809 as we're turning on the status subresource by default which is beta in k8s 1.11 per kubernetes/kubernetes#63598
Although I would like to see a manual confirmation of the status subresource being on by default (and not behind a feature gate) on a k8s 1.11 cluster.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2018
@dymurray
Copy link
Contributor Author

dymurray commented Dec 5, 2018

@hasbro17 I can confirm that bringing up a v1.11.0 Kubernetes Cluster the status subresource is on by default.

@joelanford
Copy link
Member

@dymurray Have you looked into the Helm error in this build at all? I'm seeing the same error trying to add a status subresource to a CRD.

https://travis-ci.org/operator-framework/operator-sdk/builds/464000402?utm_source=github_status&utm_medium=notification

@joelanford
Copy link
Member

I think it's a bug fixed in 1.11.3 here, but I'm not positive. With 1.11.2 the helm e2e test works if I change the operator-sdk new command to --kind=Memcached. With 1.11.3, the helm e2e test works as is.

@dymurray
Copy link
Contributor Author

dymurray commented Dec 5, 2018

@joelanford I haven't dug into it just yet but your link looks promising. I will report back if I find anything. It only began happening when I added the status writer to the helm controller.

@shawn-hurley
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2018
doc/user/client.md Outdated Show resolved Hide resolved
@hasbro17
Copy link
Contributor

hasbro17 commented Dec 5, 2018

@dymurray Can you please update the PR title and description to more clearly say that we're enabling the status subresource by default.

@dymurray dymurray changed the title pkg/ansible/controller; Support for hybrid user-driven status pkg/ansible/controller,pkg/helm/controller; Enable status subresource by default Dec 6, 2018
@dymurray
Copy link
Contributor Author

dymurray commented Dec 6, 2018

Helm CI tests are related to the bug Joe mentioned above. I ran the tests with kubectl version 1.11.3 and the error went away.

Edit: I bumped the client libraries to 1.11.5 to resolve this and updated Gopkg.* files

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2018
@openshift-ci-robot openshift-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 6, 2018
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2018
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2018
@dymurray
Copy link
Contributor Author

dymurray commented Dec 7, 2018

I have removed all client library bumps from this PR because of #807

Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

LGTM

@hasbro17
Copy link
Contributor

hasbro17 commented Dec 7, 2018

Overall SGTM
@dymurray Can you please include a line in the CHANGELOG under the Changed section to mention that the SDK now generates the CRD with the status subresource enabled by default.
And link to doc/user/client.md doc on how to update the status.

@dymurray
Copy link
Contributor Author

Thanks @hasbro17, added the note to the changelog.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM after nit

@hasbro17 hasbro17 merged commit 2903644 into operator-framework:master Dec 11, 2018
@hasbro17
Copy link
Contributor

My bad, I forgot to squash before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language/ansible Issue is related to an Ansible operator project needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable the status subresource for k8s 1.12
7 participants