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

🌱 Switch workload/namespace controller to use committer #2874

Merged
merged 2 commits into from
Mar 31, 2023
Merged

🌱 Switch workload/namespace controller to use committer #2874

merged 2 commits into from
Mar 31, 2023

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Mar 7, 2023

Summary

  1. Switch committer to allow changes to both spec/meta and status in the same reconcile loop. If both were changed, only the spec/meta patch is sent. If nothing else changes, the presumption is the status patch will go through on the next reconcile iteration.
  2. Switch workload/namespace controller to use committer. This involved removing the individual places where it was sending namespace patches directly and replacing them with calculating the desired state of the namespace in the reconcile logic. Then, after reconcile() is done, the committer will send a patch if needed.

Related issue(s)

Part of #2635

@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 7, 2023
@ncdc
Copy link
Member Author

ncdc commented Mar 7, 2023

@stevekuznetsov curious for your thoughts on this

@ncdc ncdc force-pushed the committer/workload/namespace branch from c4bc9d8 to 18ec4de Compare March 7, 2023 19:03
@ncdc ncdc mentioned this pull request Mar 7, 2023
Closed
20 tasks
sttts
sttts reviewed Mar 10, 2023
View reviewed changes
pkg/apis/scheduling/v1alpha1/types_location.go Outdated
@@ -25,7 +25,8 @@ const (
// representation of the location labels in order to use them in a table column in the CLI.
LocationLabelsStringAnnotationKey = "scheduling.kcp.io/labels"

// PlacementAnnotationKey is the label key for the label holding a PlacementAnnotation struct.
// PlacementAnnotationKey is the label key for a namespace that indicates the namespace's labels match the selector
// in at least one ready Placement.
Copy link
Member

Choose a reason for hiding this comment

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

this drops the specification of the data format. Has this become a string and not a struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

As best I could tell, it was never a struct.

Copy link
Member

Choose a reason for hiding this comment

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

@davidfestal @qiujian16 @jmprusi can one of you clarify?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked everywhere and this is the only place "PlacementAnnotation" is mentioned. Also the code only ever sets the empty string and checks for presence.

@ncdc
Copy link
Member Author

ncdc commented Mar 10, 2023

/hold

I can make this better next week 😀

@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 Mar 10, 2023
@ncdc ncdc force-pushed the committer/workload/namespace branch from 18ec4de to 9096eea Compare March 13, 2023 15:58
@ncdc
Copy link
Member Author

ncdc commented Mar 13, 2023

@sttts I've reverted my committer changes and restored the reconcile status continue/stop logic, but have still converted this to use committer instead of issuing patches directly. PTAL!

@ncdc
Copy link
Member Author

ncdc commented Mar 13, 2023

/unhold

@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 Mar 13, 2023
@ncdc ncdc added the area/transparent-multi-cluster Related to scheduling of workloads into pclusters. label Mar 14, 2023
@ncdc ncdc mentioned this pull request Nov 23, 2023
Open
@ncdc ncdc force-pushed the committer/workload/namespace branch from 9096eea to 7547f60 Compare March 14, 2023 18:34
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2023
@sttts
Copy link
Member

sttts commented Mar 31, 2023

/approve
/lgtm

needs rebase

@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 Mar 31, 2023
@ncdc
Correct label comment
8eacb11 
Signed-off-by: Andy Goldstein <[email protected]>
@ncdc
workload/namespace: use committer
35e42f5 
Signed-off-by: Andy Goldstein <[email protected]>
@ncdc ncdc force-pushed the committer/workload/namespace branch from 539f493 to 35e42f5 Compare March 31, 2023 14:09
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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

@sttts
Copy link
Member

sttts commented Mar 31, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2023
@openshift-merge-robot openshift-merge-robot merged commit 530d15f into kcp-dev:main Mar 31, 2023
@kcp-ci-bot kcp-ci-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@qiujian16 qiujian16 Awaiting requested review from qiujian16

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/transparent-multi-cluster Related to scheduling of workloads into pclusters. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ncdc @sttts @stevekuznetsov @openshift-merge-robot @kcp-ci-bot