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.

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

Get rid of server-side apply #139

Merged
merged 3 commits into from Nov 25, 2021
Merged

Get rid of server-side apply #139

merged 3 commits into from Nov 25, 2021

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Oct 25, 2021

Currently the operator uses Server-Side Apply (SSA)[1] to manage its resources. Unfortunately, this approach has some drawbacks as it allows users to modify resources independently from the operator.

To prevent this behavior, this commit adds custom apply functions inspired by ones from resourseapply module in library-go[2].

[1] https://kubernetes.io/docs/reference/using-api/server-side-apply/
[2] https://github.com/openshift/library-go/tree/master/pkg/operator/resource/resourceapply

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2021
@Fedosin Fedosin force-pushed the no_ssa branch 2 times, most recently from f4ea437 to c8cc989 Compare October 25, 2021 13:36
@Fedosin Fedosin changed the title WIP: Get rid of server-side apply Get rid of server-side apply Oct 25, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2021
elmiko
elmiko reviewed Oct 25, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this mostly makes sense to me, just a question about the ConfigMap function

pkg/controllers/resourceapply.go Outdated
}

// ApplyConfigMap merges objectmeta, requires data
func ApplyConfigMap(ctx context.Context, client coreclientv1.Client, recorder record.EventRecorder, required *corev1.ConfigMap) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do ConfigMaps not use the spechash annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, in OpenShift they don't have it: https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/core.go#L159-L235
I decided to keep it consistent with the library-go implementation.

JoelSpeed
JoelSpeed reviewed Oct 28, 2021
View reviewed changes
pkg/controllers/resourceapply.go Outdated
@@ -0,0 +1,210 @@
package controllers
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth moving this file to a subpackage called resourceapply to match library-go?

Are there any tests we can copy over from library go?

Would also be good to have links to the original source of these functions

If we are keeping it within this package, these functions do not need to be exported, please make them private

pkg/controllers/clusteroperator_controller_test.go Outdated
Comment on lines 675 to 682
// Manually changing the port number
ports := []corev1.ContainerPort{
{
ContainerPort: 11258,
Name: "http",
Protocol: corev1.ProtocolTCP,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not demonstrate the change that you've introduced within this PR. As the port number is specified in the resource already, the server side apply would have overwritten this value.

I would suggest adding a secondary port and then using the test to expect that the extra port is then removed.

At the moment this test will pass with or without your code changes

pkg/controllers/clusteroperator_controller_test.go Outdated
Eventually(recorder.Events).Should(Receive(ContainSubstring("Resource was successfully updated")))

// Manually inserting a new label
dep.Labels["my-label"] = "someValue"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer to use constants for these so that we don't have to declare them twice, here and on line 764

pkg/controllers/clusteroperator_controller_test.go
Expect(dep.Labels["my-label"]).To(Equal("someValue"))
})

It("Expect to have modified system label reverted back", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should also pass with the SSA implementation, do we not have something covering this already?

@Fedosin Fedosin force-pushed the no_ssa branch 2 times, most recently from b948e2d to 56fe92b Compare November 5, 2021 19:47
lobziik
lobziik reviewed Nov 9, 2021
View reviewed changes
pkg/controllers/clusteroperator_controller.go Outdated
} else if err != nil {
r.Recorder.Event(resource, corev1.EventTypeWarning, "Update failed", err.Error())
return false, err
switch t := resource.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about moving this switch to 'resourceapply' package as a separate function. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@Fedosin Fedosin force-pushed the no_ssa branch 3 times, most recently from 50ec09b to 03e5236 Compare November 23, 2021 20:08
@Fedosin
Copy link
Contributor Author

Fedosin commented Nov 23, 2021

/retest

@JoelSpeed
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2021
lobziik
lobziik reviewed Nov 24, 2021
View reviewed changes
Copy link
Contributor

@lobziik lobziik left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one question about exporting the whole bunch of different fuctions from resourceapply module. Might it make sense to export only ApplyResource out of it?

pkg/controllers/resourceapply/resourceapply.go Outdated Show resolved Hide resolved
@lobziik
Copy link
Contributor

lobziik commented Nov 24, 2021

/test e2e-azure-ccm

@Fedosin
Add resourceapply module
3fc9247
@Fedosin
Add tests to comply with OpenShift requirements
381d695 
These tests allow to make sure that the operator applies the resources
as required by OpenShift: users are allowed to add custom labels and
annotations, but everything else will be overwritten by the operator.

Since we use Server-Side Apply mechanism that sometimes doesn't comply
with these requirements, it should be replaced to prevent collisions.
@lobziik
Copy link
Contributor

lobziik commented Nov 25, 2021

/lgtm

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

openshift-ci bot commented Nov 25, 2021 •
edited

@Fedosin: 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/e2e-vsphere-ccm 381d695 link false /test e2e-vsphere-ccm
ci/prow/e2e-vsphere-ccm-install 381d695 link false /test e2e-vsphere-ccm-install
ci/prow/e2e-azure 381d695 link false /test e2e-azure
ci/prow/e2e-gcp-ccm-install 381d695 link false /test e2e-gcp-ccm-install
ci/prow/e2e-gcp-ccm 381d695 link false /test e2e-gcp-ccm

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

/retest-required

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

8 similar comments
@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/retest-required

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

@openshift-bot
Copy link

/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 1aa4c6e into openshift:master Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elmiko elmiko elmiko left review comments

@lobziik lobziik lobziik left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

Assignees

@lobziik lobziik

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Fedosin @JoelSpeed @lobziik @openshift-bot @elmiko @openshift-merge-robot