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

Promote Statefulset controller and its e2e tests to use apps/v1 #55792

Merged
merged 1 commit into from
Jan 26, 2018
Merged

Promote Statefulset controller and its e2e tests to use apps/v1 #55792

merged 1 commit into from
Jan 26, 2018

Conversation

dhilipkumars
Copy link

What this PR does / why we need it:
Promotes the statefulset controller to use to use the latest apps group apps/v1

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # #55714

Special notes for your reviewer:

  • Listerexpansion for v1 k8s.io/client-go/listers/apps/v1 (was recently done for v1beta2)

  • v1beta2 && v1 had ObservedGeneration as int64 where as v1beta1 and rest of the code (including conversion) is expecting ObservedGeneration to be *int64

type StatefulSetStatus struct {
	// observedGeneration is the most recent generation observed for this StatefulSet. It corresponds to the
	// StatefulSet's generation, which is updated on mutation by the API Server.
	// +optional
	ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,1,opt,name=observedGeneration"`
  • for kubectl's rollback and history commands a couple functions have been duplicated to allow us to use v1 version instead of v1beta1 for statefulsets, while the older functions are still used by other controllers.

We should be able to remove these duplicates once all the controllers are moved.

If this aligns with the plan then i could move other controllers too.

cc: @kow3ns

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 15, 2017
@dhilipkumars
Copy link
Author

/sig apps
@kubernetes/sig-apps-api-reviews

@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 15, 2017
@dhilipkumars
Copy link
Author

/assign @kow3ns

@dhilipkumars dhilipkumars force-pushed the statefulset-appsv1 branch 5 times, most recently from 6577306 to aaf3b6a Compare November 15, 2017 15:17
@ncdc
Copy link
Member

ncdc commented Nov 15, 2017

Do we need to worry about this potentially breaking GKE (if GKE doesn't have apps/v1 yet)?

@dhilipkumars
Copy link
Author

/test pull-kubernetes-e2e-kops-aws

@dhilipkumars
Copy link
Author

/test pull-kubernetes-e2e-gce

@dhilipkumars
Copy link
Author

/test pull-kubernetes-node-e2e

@dhilipkumars
Copy link
Author

/retest

@dhilipkumars
Copy link
Author

dhilipkumars commented Nov 16, 2017 •
edited
Loading

@ncdc
"Do we need to worry about this potentially breaking GKE (if GKE doesn't have apps/v1 yet)?"

This seems to paas e2e-gke-gci now is it good enough?

Sorry just noticing this test is skipped.

kow3ns
kow3ns reviewed Nov 16, 2017
View reviewed changes
Copy link
Member

@kow3ns kow3ns left a comment

Choose a reason for hiding this comment

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

  1. The title of this PR is confusing. StatefulSet is promoted to v1. What you are doing is converting the controller to use the v1 client.
  2. You can't do this now. If we merge this PR we break cluster upgrade for any cluster with multiple masters, and rolling back an upgrade becomes hazardous. We can accept a PR like this for the 1.10 release.

@kow3ns kow3ns added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 16, 2017
@dhilipkumars dhilipkumars changed the title Promote Statefulset controller and its e2e to apps/v1 Promote Statefulset controller and its e2e tests to use apps/v1 Nov 17, 2017
@dhilipkumars
Copy link
Author

@kow3ns Sure, I can maintain this PR until 1.9 is cut out of master, in the meanwhile could we mark this to 1.10 milestone, please?

@dhilipkumars
Copy link
Author

/test pull-kubernetes-unit

@ncdc ncdc added this to the v1.10 milestone Nov 17, 2017
@tallclair
Copy link
Member

/unassign

@kow3ns
Copy link
Member

kow3ns commented Nov 18, 2017

@dhilipkumars don't close it. We will do this in the 1.10 timeframe and we can review and merge when we come out of code freeze.

@dhilipkumars
Copy link
Author

@antoineco Conditions are already a part of v1 api object? Is there a specific functionality we should include when you say 'condition management' ?

@antoineco
Copy link
Contributor

antoineco commented Jan 9, 2018 •
edited
Loading

@dhilipkumars I meant actually computing the resource conditions at the end of the reconciliation loop. Currently it seems like the sts API can take conditions, but nothing is generating them.

@dhilipkumars
Copy link
Author

dhilipkumars commented Jan 9, 2018 •
edited
Loading

@antoineco I'm not sure that is the scope of this PR, this is simply making the core controller work on top of v1 type objects instead of v1beta.. objects.

Could you please create an issue if there isn't one so that this requirement can be tracked?

@antoineco antoineco mentioned this pull request Jan 9, 2018
Closed
@dhilipkumars
Copy link
Author

ping @kubernetes/sig-apps-api-reviews

@dhilipkumars dhilipkumars force-pushed the statefulset-appsv1 branch 2 times, most recently from 69a30b7 to 0de0e5f Compare January 10, 2018 17:51
dixudx
dixudx suggested changes Jan 11, 2018
View reviewed changes
pkg/apis/apps/v1/conversion.go Outdated
@@ -453,8 +453,10 @@ func Convert_apps_StatefulSetUpdateStrategy_To_v1_StatefulSetUpdateStrategy(in *
}

func Convert_v1_StatefulSetStatus_To_apps_StatefulSetStatus(in *appsv1.StatefulSetStatus, out *apps.StatefulSetStatus, s conversion.Scope) error {
out.ObservedGeneration = new(int64)
*out.ObservedGeneration = in.ObservedGeneration
if in.ObservedGeneration != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This does not make sense.
in. ObservedGeneration is int64, which cannot be nil.
I prefer to keep this unchanged.

Copy link
Author

Choose a reason for hiding this comment

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

@dixudx v1beta1 still uses *int64. It is inconsistent across api versions which one should we follow?

Copy link
Member

Choose a reason for hiding this comment

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

Refer to #49607. Don't change the type back.

pkg/apis/apps/v1/conversion.go Outdated
@@ -476,7 +478,8 @@ func Convert_v1_StatefulSetStatus_To_apps_StatefulSetStatus(in *appsv1.StatefulS

func Convert_apps_StatefulSetStatus_To_v1_StatefulSetStatus(in *apps.StatefulSetStatus, out *appsv1.StatefulSetStatus, s conversion.Scope) error {
if in.ObservedGeneration != nil {
out.ObservedGeneration = *in.ObservedGeneration
out.ObservedGeneration = new(int64)
Copy link
Member

Choose a reason for hiding this comment

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

out.ObservedGeneration is type int64. Should not create a new int64 ptr here.
Keep this unchanged.

Actually there are inconsistence between apps/internal and apps/v1. Such as ObservedGeneration is type *int64 in apps/internal, while it is changed to int64 in apps/v1.

@kow3ns kow3ns removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jan 11, 2018
@dhilipkumars
Copy link
Author

/test pull-kubernetes-cross

dixudx
dixudx reviewed Jan 14, 2018
View reviewed changes
pkg/apis/apps/v1/conversion.go Outdated
@@ -453,6 +453,7 @@ func Convert_apps_StatefulSetUpdateStrategy_To_v1_StatefulSetUpdateStrategy(in *
}

func Convert_v1_StatefulSetStatus_To_apps_StatefulSetStatus(in *appsv1.StatefulSetStatus, out *apps.StatefulSetStatus, s conversion.Scope) error {

Copy link
Member

Choose a reason for hiding this comment

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

Remove this newline.

pkg/apis/apps/v1/conversion.go Outdated
@@ -472,9 +473,11 @@ func Convert_v1_StatefulSetStatus_To_apps_StatefulSetStatus(in *appsv1.StatefulS
}
}
return nil

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

pkg/apis/apps/v1/conversion.go Outdated
}

func Convert_apps_StatefulSetStatus_To_v1_StatefulSetStatus(in *apps.StatefulSetStatus, out *appsv1.StatefulSetStatus, s conversion.Scope) error {

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

pkg/controller/statefulset/stateful_set_control.go Outdated
@@ -22,7 +22,8 @@ import (

"github.com/golang/glog"

apps "k8s.io/api/apps/v1beta1"
apps "k8s.io/api/apps/v1"
//apps "k8s.io/api/apps/v1beta1"
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to remove this?

pkg/controller/statefulset/stateful_set_utils.go Outdated
@@ -23,7 +23,8 @@ import (
"regexp"
"strconv"

apps "k8s.io/api/apps/v1beta1"
//apps "k8s.io/api/apps/v1beta1"
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to remove this ?

pkg/kubectl/rollback.go Outdated
return "", fmt.Errorf("failed restoring revision %d: %v", toRevision, err)
}

return rollbackSuccess, nil
}

// findHistoryV1 returns a controllerrevision of a specific revision from the given controllerrevisions.
// It returns nil if no such controllerrevision exists.
// If toRevision is 0, the last previously used history is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Add a TODO here to rename this to findHistory when other controllers have been upgraded.

test/e2e/apps/statefulset.go Outdated
apps "k8s.io/api/apps/v1beta1"
appsv1beta2 "k8s.io/api/apps/v1beta2"
apps "k8s.io/api/apps/v1"
//appsv1beta1 "k8s.io/api/apps/v1beta1"
Copy link
Member

Choose a reason for hiding this comment

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

Remove these two comment lines.

test/e2e/apps/statefulset.go Outdated
@@ -844,7 +845,7 @@ var _ = SIGDescribe("StatefulSet", func() {
return nil
}, framework.StatefulPodTimeout, 2*time.Second).Should(BeNil())
})

/* Comment it for now.
Copy link
Member

Choose a reason for hiding this comment

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

Any reasons to comment it?

@dixudx
Copy link
Member

dixudx commented Jan 18, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2018
@kow3ns
Copy link
Member

kow3ns commented Jan 19, 2018

/approve

@dhilipkumars
Copy link
Author

cc: @smarterclayton could you please approve.

@dhilipkumars
Copy link
Author

/assign @smarterclayton

@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhilipkumars, dixudx, kow3ns, smarterclayton

Associated issue: #55714

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2018
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 26, 2018 •
edited
Loading

@dhilipkumars: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-unit aba725a link /test pull-kubernetes-unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55792, 58342). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit c21173d into kubernetes:master Jan 26, 2018
@dixudx dixudx mentioned this pull request Jan 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kow3ns kow3ns kow3ns left review comments

@dixudx dixudx dixudx requested changes

Assignees

@ncdc ncdc

@smarterclayton smarterclayton

@dixudx dixudx

@kow3ns kow3ns

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-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
10 participants
@dhilipkumars @ncdc @tallclair @kow3ns @mattfarina @antoineco @dixudx @smarterclayton @k8s-ci-robot @k8s-github-robot