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

Added field CollisionCount to StatefulSetStatus #49983

Merged
merged 4 commits into from
Aug 11, 2017

Conversation

liyinan926
Copy link
Contributor

@liyinan926 liyinan926 commented Aug 1, 2017

What this PR does / why we need it:
This PR added a new field CollisionCount into StatefulSetStatus, similarly in terms of both name and semantics to the existing CollisionCount field in DaemonSetStatus. The field will be used for collision avoidance when the StatefulSet controller creates name for the newest ControllerRevision, which will be done in another PR.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): #49909.

Special notes for your reviewer:
A second PR will include logic that actually uses the field for collision avoidance.

Release note:

Added field CollisionCount to StatefulSetStatus in both apps/v1beta1 and apps/v1beta2

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 1, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @liyinan926. 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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 1, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 1, 2017
@david-mcmahon david-mcmahon removed their assignment Aug 1, 2017
@liyinan926
Copy link
Contributor Author

CLA signed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 1, 2017
@@ -187,6 +187,12 @@ type StatefulSetStatus struct {
// updateRevision, if not empty, indicates the version of the StatefulSet used to generate Pods in the sequence
// [replicas-updatedReplicas,replicas)
UpdateRevision string

// Count of hash collisions for the StatefulSet. The StatefulSet controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the code style, like "collisionCount is the count of hash collisions for the StatefulSet ..."
Please also fix the code style in other places for this field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 2, 2017
@kow3ns
Copy link
Member

kow3ns commented Aug 2, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 2, 2017
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 2, 2017
@liyinan926
Copy link
Contributor Author

/retest

@liyinan926
Copy link
Contributor Author

/assign @thockin

@liyinan926
Copy link
Contributor Author

/assign thockin

@liyinan926
Copy link
Contributor Author

@thockin Need your approval. PTAL. Thanks!

// uses this field as a collision avoidance mechanism when it needs to create the name for the
// newest ControllerRevision.
// +optional
CollisionCount *int64
Copy link
Member

Choose a reason for hiding this comment

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

Why is this int64 and not int32? Do you expect more than 2 billion collisions? JSON is known to have trouble carrying int64 values, which can make automated API fuzzing difficult.

Copy link
Contributor Author

@liyinan926 liyinan926 Aug 7, 2017

Choose a reason for hiding this comment

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

This is for consistency across controllers. It's the type of the field used by other controllers, e.g., here and here.

Copy link
Member

Choose a reason for hiding this comment

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

Consistent with wrong is still wrong. I'd like to hear a reason this can't be int32, and the others fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update to use int32 and have a follow up PR to change it to int32 in other controllers.

Copy link
Member

Choose a reason for hiding this comment

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

@thockin

JSON is known to have trouble carrying int64 values, which can make automated API fuzzing difficult.

This is not an issue that users have actually reported having, and fuzzing currently works for these API objects. If you configure your decoder appropriately or use structs, or classes depending on the language, to decode, disambiguating int64 values is definitely achievable without much work.

Given that the API for Deployment and DeamonSet both went through community design review prior to implementation and merge, that there is no measurable performance benefit to using an int32 in this case, that we use int64 in many other places in the API, and that we don't have an actual issue to motivate changing all the workloads API objects, I don't see why we should switch this field to int32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thockin What are your thoughts on the comment @kow3ns made above?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed. We should convert to int32 in bulk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM. Will have a PR to convert all.

@liyinan926
Copy link
Contributor Author

/test pull-kubernetes-unit

@liyinan926
Copy link
Contributor Author

@thockin replied to your comment. PTAL. Thanks!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2017
@liyinan926
Copy link
Contributor Author

@kow3ns need lgtm again after rebasing. @thockin need you approval. PTAL. Thanks!

@kow3ns
Copy link
Member

kow3ns commented Aug 9, 2017

/retest

@kow3ns
Copy link
Member

kow3ns commented Aug 9, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017
@liyinan926
Copy link
Contributor Author

/retest

@thockin
Copy link
Member

thockin commented Aug 10, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kow3ns, liyinan926, thockin

Associated issue: 49909

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@caesarxuchao
Copy link
Member

@liyinan926 do you remember why the PR had two merge commits "Merge branch 'master' of github.com:kubernetes/kubernetes"? Did you merge your local branch with kubernetes/kubernetes master branch before submitting the PR?

@sttts reported this PR broke the publishing robot because the robot skipped merge commits while in case of this PR, the merge commits are non-empty.

@liyinan926
Copy link
Contributor Author

@caesarxuchao @sttts I don't remember exactly why but I did pulled the master branch and rebased before the PR was approved and submitted.

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Automatic merge from submit-queue

Added field CollisionCount to StatefulSetStatus

**What this PR does / why we need it**:
This PR added a new field `CollisionCount` into `StatefulSetStatus`, similarly in terms of both name and semantics to the existing `CollisionCount` field in `DaemonSetStatus`.  The field will be used for collision avoidance when the `StatefulSet` controller creates name for the newest ControllerRevision, which will be done in another PR.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: kubernetes#49909.

**Special notes for your reviewer**:
A second PR will include logic that actually uses the field for collision avoidance.

**Release note**:
```release-note
Added field CollisionCount to StatefulSetStatus in both apps/v1beta1 and apps/v1beta2
```
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants