Workloads V1 #53679

kow3ns · 2017-10-10T21:36:07Z

What this PR does / why we need it: This PR promotes the Deployment, ReplicaSet, and DaemonSet StatefulSet, ControllerRevision kinds to the apps/v1 group version.

kubernetes/enhancements#353

Special notes for your reviewer:
There will be at least two followups to this PR. The first to add a scale sub-resource when the correct location is resolved, and the second to deal with Conditions in the workloads API.

While it would have been preferable to move the kinds individually providing a lesser burden on reviewers, this proved impracticable due to the intricacies of version resolution in kubectl for objects of the different kinds in the same group.

DaemonSet, Deployment, ReplicaSet, and StatefulSet have been promoted to GA and are available in the apps/v1 group version.

janetkuo · 2017-10-10T21:41:52Z

What this PR does / why we need it: This PR promotes the Deployment, ReplicaSet, and DaemonSet kinds to the apps/v1 group version.

PR desc needs to be updated: replace DaemonSet with StatefulSet. Need to remove DS from release note too since it's already done.

kow3ns · 2017-10-10T21:54:29Z

StatefulSet is there, but we will probably do a single release note for the entire API.

kow3ns · 2017-10-10T23:41:07Z

/retest

kow3ns · 2017-10-11T00:00:58Z

@liggitt this version is without the scale
@kubernetes/sig-api-machinery-api-reviews
@smarterclayton
@bgrant0607

kow3ns · 2017-10-11T03:04:35Z

/test pull-kubernetes-unit

bgrant0607 · 2017-10-11T03:26:50Z

@kow3ns I thought we were going to do this in a branch?

kow3ns · 2017-10-11T03:29:56Z

@bgrant0607 we are still trying to get the branch working and we didn't want to stop all development until then. The branch still can't run tests.

kow3ns · 2017-10-11T03:30:20Z

/test pull-kubernetes-unit

kow3ns · 2017-10-11T04:06:27Z

/retest

kow3ns · 2017-10-11T14:11:18Z

/retest

janetkuo · 2017-10-11T20:20:53Z

pkg/apis/apps/v1/defaults.go

+// - RevisionHistoryLimit set to 10 (not set in extensions)
+// - ProgressDeadlineSeconds set to 600s (not set in extensions)
+func SetDefaults_Deployment(obj *appsv1.Deployment) {
+	// Set appsv1beta2.DeploymentSpec.Replicas to 1 if it is not set.


nit: "appsv1"

Good catch. I'm going to rebase the update into the first commit on the PR.

janetkuo · 2017-10-11T20:33:03Z

hack/make-rules/test-cmd-util.sh

@@ -1350,7 +1350,7 @@ run_kubectl_get_tests() {
  kube::test::if_has_string "${output_message}" "/api/v1/namespaces/default/pods 200 OK"
  kube::test::if_has_string "${output_message}" "/api/v1/namespaces/default/replicationcontrollers 200 OK"
  kube::test::if_has_string "${output_message}" "/api/v1/namespaces/default/services 200 OK"
-  kube::test::if_has_string "${output_message}" "/apis/apps/v1beta1/namespaces/default/statefulsets 200 OK"
+  kube::test::if_has_string "${output_message}" "/apis/apps/v1/namespaces/default/statefulsets 200 OK"


Would this test be running against clusters that don't have apps/v1 enabled yet?

The command tests are always written expecting a specific version of group version kind to be returned. That version has always been dependent on the packaged kubectl and apiserver configuration.

janetkuo · 2017-10-11T20:34:06Z

Mention that ControllerRevision is bumped to apps/v1 in PR title/desc and release note too.

janetkuo · 2017-10-12T00:54:19Z

pkg/kubectl/cmd/util/factory_object_mapping.go

@@ -330,19 +330,21 @@ func (f *ring1Factory) Reaper(mapping *meta.RESTMapping) (kubectl.Reaper, error)
 func (f *ring1Factory) HistoryViewer(mapping *meta.RESTMapping) (kubectl.HistoryViewer, error) {
 	mappingVersion := mapping.GroupVersionKind.GroupVersion()
 	clientset, err := f.clientAccessFactory.ClientSetForVersion(&mappingVersion)


check the returned err

janetkuo · 2017-10-12T00:58:55Z

pkg/kubectl/history.go

 	}
 	return nil, fmt.Errorf("no history viewer has been implemented for %q", kind)
 }

 type DeploymentHistoryViewer struct {
-	c clientset.Interface
+	ic internalclientset.Interface
+	ec kubernetes.Interface


kubernetes.Clienset?

We don't need any implementation detail in this context so I think we should go with interface.

+1 for interface

janetkuo · 2017-10-12T00:59:05Z

pkg/kubectl/history.go

@@ -147,14 +149,15 @@ func printTemplate(template *v1.PodTemplateSpec) (string, error) {
 }

 type DaemonSetHistoryViewer struct {
-	c clientset.Interface
+	id internalclientset.Interface


not sure how much is preexisting, but kubectl should be moving toward dealing in external versions
cc @kubernetes/sig-cli-pr-reviews

I'm just going to go ahead and move the rollbackers and history to external version (v1beta1) and unwire the internal. We know the client is moving in this direction, and we might as well help the process along while there is time.

janetkuo · 2017-10-12T01:03:07Z

pkg/kubectl/history.go

@@ -52,26 +53,27 @@ type HistoryViewer interface {
 	ViewHistory(namespace, name string, revision int64) (string, error)
 }



It looks like we don't need internal clientset at all in history viewer, do we?

janetkuo · 2017-10-12T01:04:13Z

pkg/kubectl/rollback.go

@@ -54,20 +55,21 @@ type Rollbacker interface {
 	Rollback(obj runtime.Object, updatedAnnotations map[string]string, toRevision int64, dryRun bool) (string, error)
 }



We probably don't need ic here either

janetkuo · 2017-10-12T01:04:46Z

pkg/kubectl/cmd/util/factory_object_mapping.go

 	if err != nil {
 		return nil, err
 	}
-	return kubectl.HistoryViewerFor(mapping.GroupVersionKind.GroupKind(), clientset)
+	return kubectl.HistoryViewerFor(mapping.GroupVersionKind.GroupKind(), clientset, external)
 }

 func (f *ring1Factory) Rollbacker(mapping *meta.RESTMapping) (kubectl.Rollbacker, error) {
 	mappingVersion := mapping.GroupVersionKind.GroupVersion()
 	clientset, err := f.clientAccessFactory.ClientSetForVersion(&mappingVersion)


check err

janetkuo · 2017-10-12T01:06:36Z

pkg/kubectl/cmd/util/factory_object_mapping.go

@@ -330,19 +330,21 @@ func (f *ring1Factory) Reaper(mapping *meta.RESTMapping) (kubectl.Reaper, error)
 func (f *ring1Factory) HistoryViewer(mapping *meta.RESTMapping) (kubectl.HistoryViewer, error) {
 	mappingVersion := mapping.GroupVersionKind.GroupVersion()
 	clientset, err := f.clientAccessFactory.ClientSetForVersion(&mappingVersion)
+	external, err := f.clientAccessFactory.KubernetesClientSet()


naming nit: if we need both internal and external clientset, name the internal one "internal" or name the external one "externalClientset"

kow3ns · 2017-10-12T18:52:38Z

@janetkuo @liggitt I made the modification to remove the v1beta2 documentation reference in Defaults. I unwired the unversioned clientset as well. Both commits are squashed, the former into the api modification commit and the latter into the kubectl commit.

janetkuo · 2017-11-01T23:58:26Z

/lgtm

krmayankk · 2017-11-02T09:01:59Z

@kow3ns what would be needed for users to migrate from v1beta1 to v1 for these objects ?

kow3ns · 2017-11-02T16:40:04Z

@krmayankk the storage formats are not changed. You can consume the API from apps/v1. The deprecation cycle for extensions/v1, apps/v1beta1, and apps/v1beta2 will take into account that users are currently depending on them in production. Subsequent releases will include instructions on how to upgrade storage formats in order to migrate existing workloads to a newer version.

…Set kinds to the apps/v1 group version.

…fulSet History and Rollback and updates test-cmd for new versions

liggitt · 2017-11-03T17:50:37Z

test/integration/etcd/etcd_storage_path_test.go

+		expectedGVK:      gvkP("extensions", "v1beta1", "ReplicaSet"),
+	},
+	gvr("apps", "v1", "controllerrevisions"): {
+		stub:             `{"metadata":{"name":"crs3"},"data":{"name":"abc","namespace":"default","creationTimestamp":null,"Spec":{"Replicas":0,"Selector":{"matchLabels":{"foo":"bar"}},"Template":{"creationTimestamp":null,"labels":{"foo":"bar"},"Spec":{"Volumes":null,"InitContainers":null,"Containers":null,"RestartPolicy":"Always","TerminationGracePeriodSeconds":null,"ActiveDeadlineSeconds":null,"DNSPolicy":"ClusterFirst","NodeSelector":null,"ServiceAccountName":"","AutomountServiceAccountToken":null,"NodeName":"","SecurityContext":null,"ImagePullSecrets":null,"Hostname":"","Subdomain":"","Affinity":null,"SchedulerName":"","Tolerations":null,"HostAliases":null}},"VolumeClaimTemplates":null,"ServiceName":""},"Status":{"ObservedGeneration":null,"Replicas":0}},"revision":0}`,


this stub is using JSON generated from an internal object (note the capitalized keys). this is pre-existing in the other stub tests and can be fixed as a follow up.

liggitt · 2017-11-03T18:14:21Z

/approve

pwittrock · 2017-11-03T21:01:25Z

/approve

approve for hack

foxish · 2017-11-03T21:03:56Z

/lgtm

k8s-github-robot · 2017-11-03T21:04:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: foxish, janetkuo, kow3ns, liggitt, pwittrock

Associated issue: 353

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

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

~~api/OWNERS~~ [liggitt]
~~cmd/kube-apiserver/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt,pwittrock]
~~hack/OWNERS~~ [pwittrock]
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/kubectl/OWNERS~~ [janetkuo,liggitt,pwittrock]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~test/OWNERS~~ [foxish,janetkuo,kow3ns,liggitt,pwittrock]

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 · 2017-11-03T21:33:30Z

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

k8s-github-robot · 2017-11-03T22:17:15Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@kow3ns

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Promote Statefulset controller and its e2e tests to use apps/v1 **What this PR does / why we need it**: Promotes the statefulset controller to use to use the latest apps group [apps/v1](#53679) **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**: ```release-note NONE ```

kow3ns assigned janetkuo Oct 10, 2017

kow3ns requested a review from janetkuo October 10, 2017 21:36

k8s-ci-robot added 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 10, 2017

kow3ns force-pushed the workloadsv1 branch from 1026f9e to 13a2f92 Compare October 10, 2017 21:52

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Oct 11, 2017

kow3ns force-pushed the workloadsv1 branch from 13a2f92 to 9549174 Compare October 11, 2017 02:37

kow3ns force-pushed the workloadsv1 branch from 9549174 to 4644337 Compare October 11, 2017 05:45

This was referenced Oct 11, 2017

Bump statefulset to apps/v1 #53415

Closed

Bump replicaset to apps/v1 #53351

Closed

janetkuo reviewed Oct 11, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2017

janetkuo reviewed Oct 12, 2017

View reviewed changes

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Oct 12, 2017

kow3ns force-pushed the workloadsv1 branch from 4644337 to a40df20 Compare October 12, 2017 18:43

kow3ns force-pushed the workloadsv1 branch from a40df20 to cd055dd Compare October 12, 2017 19:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2017

kow3ns assigned lavalamp Nov 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2017

Kenneth Owens added 3 commits November 2, 2017 14:19

Promotes the StatefulSet, ControllerRevision, Deployment, and Replica…

26bf978

…Set kinds to the apps/v1 group version.

autogenerated code

7c772ab

gets the correct version of kubernetes client for DaemonSet and State…

5590c1f

…fulSet History and Rollback and updates test-cmd for new versions

kow3ns force-pushed the workloadsv1 branch from 6bfadac to 5590c1f Compare November 3, 2017 17:17

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 Nov 3, 2017

liggitt reviewed Nov 3, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017

k8s-ci-robot assigned foxish Nov 3, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2017

k8s-github-robot merged commit 2ecb368 into kubernetes:master Nov 3, 2017

dhilipkumars mentioned this pull request Nov 15, 2017

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

Merged

kow3ns added this to Backlog in Workloads via automation Mar 1, 2018

kow3ns moved this from Backlog to Done in Workloads Mar 1, 2018

dependabot-preview bot mentioned this pull request May 8, 2018

Bump kubernetes from 4.0.0 to 6.0.0 ministryofjustice/analytics-platform-unidler#3

Merged

dependabot-preview bot mentioned this pull request May 16, 2018

Bump kubernetes from 3.0.0 to 6.0.0 ministryofjustice/analytics-platform-control-panel#161

Closed

oomichi mentioned this pull request Feb 16, 2019

Add e2e test for scale sub-resource of statefulsets #74013

Merged

		@@ -52,26 +53,27 @@ type HistoryViewer interface {
		ViewHistory(namespace, name string, revision int64) (string, error)
		}

		@@ -54,20 +55,21 @@ type Rollbacker interface {
		Rollback(obj runtime.Object, updatedAnnotations map[string]string, toRevision int64, dryRun bool) (string, error)
		}

Workloads V1 #53679

Workloads V1 #53679

Conversation

kow3ns commented Oct 10, 2017 • edited by janetkuo

janetkuo commented Oct 10, 2017 • edited

kow3ns commented Oct 10, 2017

kow3ns commented Oct 10, 2017

kow3ns commented Oct 11, 2017

kow3ns commented Oct 11, 2017

bgrant0607 commented Oct 11, 2017

kow3ns commented Oct 11, 2017

kow3ns commented Oct 11, 2017

kow3ns commented Oct 11, 2017

kow3ns commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kow3ns commented Oct 12, 2017

janetkuo commented Nov 1, 2017

krmayankk commented Nov 2, 2017

kow3ns commented Nov 2, 2017

Choose a reason for hiding this comment

liggitt commented Nov 3, 2017

pwittrock commented Nov 3, 2017

foxish commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

kow3ns commented Oct 10, 2017 •

edited by janetkuo

janetkuo commented Oct 10, 2017 •

edited