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

[apps/v1] Core controllers and e2e to use new api types #55714

Closed
dhilipkumars opened this Issue Nov 14, 2017 · 11 comments

Comments

Projects
7 participants
@dhilipkumars
Member

dhilipkumars commented Nov 14, 2017

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature
/sig apps

What happened:
These dedicated such as v1beta1 and extension set of apis are still being used by the core contollers and e2e test.
What you expected to happen:
What is the plan to move the core controller to use the new [apps/v1] group. Ideally, we should be testing the new api types instead?

For instance, rollback is removed from deployment but these are still tested in e2e, should we not ideally
only test stuff that are currently supported?
How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:
Any update on the scale sub-resource?

Controllers that needs to be updated are:

  • Statefulset
  • Deployment
  • DaemonSet
  • ReplicaSet
@dhilipkumars

This comment has been minimized.

Show comment
Hide comment
@dhilipkumars

dhilipkumars Nov 14, 2017

Member

cc: @kow3ns
/sig apps

Member

dhilipkumars commented Nov 14, 2017

cc: @kow3ns
/sig apps

@dhilipkumars

This comment has been minimized.

Show comment
Hide comment
@kow3ns

This comment has been minimized.

Show comment
Hide comment
@kow3ns

kow3ns Nov 16, 2017

Member

@dhilipkumars We can't start work on this until 1.9 is released. This needs to go into 1.10.

Member

kow3ns commented Nov 16, 2017

@dhilipkumars We can't start work on this until 1.9 is released. This needs to go into 1.10.

@dhilipkumars

This comment has been minimized.

Show comment
Hide comment
@dhilipkumars

dhilipkumars Nov 17, 2017

Member

@kow3ns If you agree with this Plan, I could work on getting it done for other controllers by the time we cut 1.9 (Raise the PRs and keep them upto date with master). We could get this in first thing when master is ready for 1.10 development?

What do you say? Also, what is the plan for Scale subresource?

Member

dhilipkumars commented Nov 17, 2017

@kow3ns If you agree with this Plan, I could work on getting it done for other controllers by the time we cut 1.9 (Raise the PRs and keep them upto date with master). We could get this in first thing when master is ready for 1.10 development?

What do you say? Also, what is the plan for Scale subresource?

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Nov 17, 2017

Member

I think e2e from master is also used for some skew tests (we typically version gate things in e2e's for ~2 releases). cc @kubernetes/sig-testing-misc for clarification there.

Member

liggitt commented Nov 17, 2017

I think e2e from master is also used for some skew tests (we typically version gate things in e2e's for ~2 releases). cc @kubernetes/sig-testing-misc for clarification there.

@mattfarina

This comment has been minimized.

Show comment
Hide comment
@mattfarina

mattfarina Jan 2, 2018

Member

@kow3ns dhilipkumars Has this already been handled?

Member

mattfarina commented Jan 2, 2018

@kow3ns dhilipkumars Has this already been handled?

@dhilipkumars

This comment has been minimized.

Show comment
Hide comment
@dhilipkumars

dhilipkumars Jan 2, 2018

Member

@mattfarina we have raised an intial PR for statefulsets, i would like to get that reviewed and merge that. I can replicate that work for other controllers immediately after that.

Member

dhilipkumars commented Jan 2, 2018

@mattfarina we have raised an intial PR for statefulsets, i would like to get that reviewed and merge that. I can replicate that work for other controllers immediately after that.

k8s-merge-robot added a commit that referenced this issue Jan 26, 2018

Merge pull request #55792 from dhilipkumars/statefulset-appsv1
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
```
@dhilipkumars

This comment has been minimized.

Show comment
Hide comment
@dhilipkumars
Member

dhilipkumars commented Feb 19, 2018

ref ##59883

@kow3ns kow3ns added this to In Progress in Workloads Feb 26, 2018

k8s-merge-robot added a commit that referenced this issue Mar 22, 2018

Merge pull request #61367 from enisoc/apps-v1-rs
Automatic merge from submit-queue (batch tested with PRs 60980, 61273, 60811, 61021, 61367). 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>.

Use apps/v1 ReplicaSet in controller and tests.

This updates the RS/RC controller and RS integration/e2e tests to use apps/v1 ReplicaSet, as part of #55714.

It does *not* update the Deployment controller, nor its integration/e2e tests, to use apps/v1 ReplicaSet. That will be done in a separate PR (#61419) because Deployment has many more tendrils embedded throughout the system.

```release-note
Conformance: ReplicaSet must be supported in the `apps/v1` version.
```

/assign @janetkuo

k8s-merge-robot added a commit that referenced this issue Mar 28, 2018

Merge pull request #61615 from janetkuo/rm-adopt-hash
Automatic merge from submit-queue (batch tested with PRs 61790, 61808, 60339, 61615, 61757). 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>.

Deployment to stop adding pod-template-hash labels/selector on adoption

**What this PR does / why we need it**: This is a blocker for #55714, because ReplicaSet selector becomes immutable in `apps/v1`. With controller ref, Deployment's ReplicaSets and Pods can avoid fighting with each others without unique label/selector (pod-template-hash), so it's safe to stop adding hash label/selector on adoption. 

**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 #61433

**Special notes for your reviewer**: This is a behavioral change to Deployment controller that will affect all versions of Deployment APIs (`apps/v1`, `extensions/v1beta1`, `apps/v1beta1`, `apps/v1beta2`). 

**Release note**:

```release-note
Deployment will stop adding pod-template-hash labels/selector to ReplicaSets and Pods it adopts. Resources created by Deployments are not affected (will still have pod-template-hash labels/selector). 
```
@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot May 20, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot commented May 20, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-merge-robot added a commit that referenced this issue May 24, 2018

Merge pull request #61419 from enisoc/apps-v1-deploy
Automatic merge from submit-queue (batch tested with PRs 62756, 63862, 61419, 64015, 64063). 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>.

Use apps/v1 Deployment/ReplicaSet in controller and kubectl

This updates the Deployment controller and integration/e2e tests to use apps/v1, as part of #55714.

This also requires updating any other components that use the `deployment/util` package, most notably `kubectl`. That means client versions 1.11 and above will only work with server versions 1.9 and above. This is well within our client-server version skew policy of +/-1 minor version.

However, this PR *only* updates the parts of `kubectl` that used `deployment/util`. So although kubectl now requires apps/v1, it still also depends on extensions/v1beta1. Migrating other parts of kubectl to apps/v1 is beyond the scope of this PR, which was just to change the Deployment controller and fix all the fallout.

```release-note
kubectl: This client version requires the `apps/v1` APIs, so it will not work against a cluster version older than v1.9.0. Note that kubectl only guarantees compatibility with clusters that are +/-1 minor version away.
```
@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Jun 19, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot commented Jun 19, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Jul 19, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

fejta-bot commented Jul 19, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Workloads automation moved this from In Progress to Done Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment