-
Notifications
You must be signed in to change notification settings - Fork 401
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
Use library-go ApplyDeployment and ApplyDaemonset #2882
Conversation
@deads2k does this look right? |
/cc @yuqi-zhang |
61bb7cd
to
a49af7e
Compare
@mkenigs: An error was encountered querying GitHub for users with public email (rioliu@redhat.com) for bug 2034364 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
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. |
a49af7e
to
18b6e58
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkenigs 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 |
18b6e58
to
54f6961
Compare
54f6961
to
62bff8a
Compare
@mkenigs: This pull request references Bugzilla bug 2034364, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rioliu@redhat.com), skipping review request. In response to this:
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. |
pkg/operator/sync.go
Outdated
if err != nil { | ||
return err | ||
} | ||
if updated { | ||
optr.daemonsetGeneration = daemonset.Generation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this get persisted so when you restart you have it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it lasts as long as the MCO process. Are you saying it needs to persist beyond that?
pkg/operator/sync.go
Outdated
if err != nil { | ||
return err | ||
} | ||
if updated { | ||
optr.deploymentGeneration = deployment.Generation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this get persisted so when you restart you have it?
just the question about persistence to ensure that the right happens on restart. Looks good otherwise. |
example from openshiftapiserver status
|
@mkenigs and I did some investigating on storing generations properly to use for apply functions, and this is what we've determined (please correct any mistakes): Using OpenshiftAPIServer object as an example (i.e. The first issue here is, the MCO never had any object tracking daemonset/deployment object generations like this. The second issue is, the MCO doesn't actually have an equivalent object to track with. The MCO creates the following CRDS:
Of these CRDs we don't have a direct equivalent of what OpenshiftAPIServer is in the above context. The closest is ControllerConfig which is a cluster level config but only used by the MCO (mayhaps naming is not very apt), and is used by the operator pod to read cluster objects (e.g. proxy, osimage) and populate for the machine-config-controller to render into templates for the machine-config-daemons. So when that in mind, our options seem to be:
Not sure which approach is best for our case. |
Tangentially related, I keep thinking of |
I suggest creating a CRD to start the equivalent of openshiftapiserver.operator.openshift.io. This would also provide a spot for operand loglevel, operator loglevel, unsupported overrides, detailed status to add additional degraded loops. Kicking deployments and daemonsets on every process restart isn't a state we should ship. |
/bugzilla refresh The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity. |
@openshift-bot: This pull request references Bugzilla bug 2034364, which is invalid:
Comment In response to this:
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. |
62bff8a
to
7dba72b
Compare
@mkenigs: This pull request references Bugzilla bug 2034364, which is invalid:
Comment In response to this:
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. |
@mkenigs: This pull request references Bugzilla bug 2034364, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rioliu@redhat.com), skipping review request. In response to this:
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. |
Our current functions in ./lib are out of date and we should be using library-go instead The library-go functions require passing the generation of the previously applied deployment/daemonset, and this should be stored on an MCO CRD. Because this currently doesn't exist, the existing generation is passed as generation. This is no worse than how MCO's current functions work, so the library-go functions should still be used until an MCO CRD is created. Files with the now unused functions and unused helper functions were removed: resourceapply/apps.go resourcemerge/{apps.go,apps_test.go,core.go,core_test.go} Helps openshift#819
7dba72b
to
e65917a
Compare
@mkenigs: An error was encountered querying GitHub for users with public email (rioliu@redhat.com) for bug 2034364 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
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. |
/bugzilla refresh |
@wking: This pull request references Bugzilla bug 2034364, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rioliu@redhat.com), skipping review request. In response to this:
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. |
/hold |
@mkenigs: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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. |
Note: I removed the BZ as this is going to be worked on via the above jira issue and so that the underlying BZ can be closed based on the other 2 PRs that merged. |
@mkenigs: The following tests failed, say
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. |
@mkenigs: PR needs rebase. 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. |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
/lifecycle frozen |
@mkenigs: The In response to this:
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. |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. In response to this:
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. |
Use library-go ApplyDeployment and ApplyDaemonset
Our current functions in ./lib are out of date and we should be using
library-go instead
The library-go functions require passing the generation of the
previously applied deployment/daemonset, and this should be stored on an
MCO CRD. Because this currently doesn't exist, the existing generation
is passed as generation. This is no worse than how MCO's current
functions work, so the library-go functions should still be used until
an MCO CRD is created.
Files with the now unused functions and unused helper functions were
removed:
resourceapply/apps.go
resourcemerge/{apps.go,apps_test.go,core.go,core_test.go}
Helps #819
Depends #2833