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

Drop extensions internal Network* types #50327

Merged
merged 2 commits into from Aug 15, 2017
Merged

Drop extensions internal Network* types #50327

merged 2 commits into from Aug 15, 2017

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Aug 8, 2017 •
edited by thockin

Fixes #46626

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2017
sttts
sttts commented Aug 8, 2017
View reviewed changes
pkg/apis/extensions/v1beta1/zz_generated.conversion.go Outdated
@@ -365,7 +352,8 @@ func Convert_v1beta1_DaemonSetSpec_To_extensions_DaemonSetSpec(in *v1beta1.Daemo

func autoConvert_extensions_DaemonSetSpec_To_v1beta1_DaemonSetSpec(in *extensions.DaemonSetSpec, out *v1beta1.DaemonSetSpec, s conversion.Scope) error {
out.Selector = (*v1.LabelSelector)(unsafe.Pointer(in.Selector))
if err := api_v1.Convert_api_PodTemplateSpec_To_v1_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
// TODO: Inefficient conversion - can we improve it?
if err := s.Convert(&in.Template, &out.Template, 0); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looks wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And goes away after make clean.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 8, 2017
@sttts sttts force-pushed the sttts-unify-network-types branch 4 times, most recently from b4ed875 to 85dee8f Compare August 9, 2017 12:44
@sttts
Copy link
Contributor Author

sttts commented Aug 9, 2017

/retest

@sttts
Copy link
Contributor Author

sttts commented Aug 9, 2017

@kubernetes/kubernetes-build-cops verify test times out after an hour.

@shyamjvs
Copy link
Member

shyamjvs commented Aug 9, 2017

I've seen this too. Guess it's this issue - #50319

@sttts sttts mentioned this pull request Aug 10, 2017
Closed
@sttts sttts changed the title [WIP] Drop extensions internal Network* types Drop extensions internal Network* types Aug 10, 2017
@sttts
Copy link
Contributor Author

sttts commented Aug 10, 2017

/assign @thockin

@thockin is this approved?

caesarxuchao
caesarxuchao reviewed Aug 10, 2017
View reviewed changes
hack/test-update-storage-objects.sh
KUBE_OLD_API_VERSION="storage.k8s.io/v1beta1,extensions/v1beta1"
KUBE_NEW_API_VERSION="storage.k8s.io/v1,extensions/v1beta1"
KUBE_OLD_API_VERSION="networking.k8s.io/v1,storage.k8s.io/v1beta1,extensions/v1beta1"
KUBE_NEW_API_VERSION="networking.k8s.io/v1,storage.k8s.io/v1,extensions/v1beta1"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we should disable extensions/v1beta1 in the new api version, because we want to make sure we can read from networking.k8s.io/v1 even if extensions/v1beta1 is removed.

At line 111, we should add a test data for networking.k8s.io object.

@sttts you only changed the internal versions of the PR, so it's not your responsibility to add the upgrade test. It should had been added when we created networking.k8s.io/v1.

Copy link
Member

Choose a reason for hiding this comment

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

@sttts could you open a followup issue/PR for fixing this script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we still write extensions/v1beta1 objects (see discussion below), disabling extensions/v1beta1 will break networking.k8s.io/v1 as the former cannot be decoded from storage anymore. KUBE_API_VERSIONS is all or nothing: types+http-handler or no registered types and no http-handler for the disabled group. I wonder whether we shouldn't have a way to keep types registered when disabling group versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create an issue (which includes switching storage types): #50604

Copy link
Member

Choose a reason for hiding this comment

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

@sttts I think the parameter should be like this:

KUBE_OLD_API_VERSION="storage.k8s.io/v1beta1,extensions/v1beta1"
KUBE_NEW_API_VERSION="networking.k8s.io/v1,storage.k8s.io/v1"
KUBE_OLD_STORAGE_VERSIONS="extensions/v1beta1,storage.k8s.io/v1beta1"
# In step 3, networking objects will be converted from "extensions/v1beta1" and stored as "networking.k8s.io/v1".
# https://github.com/kubernetes/kubernetes/blob/v1.8.0-alpha.2/hack/test-update-storage-objects.sh#L175
KUBE_NEW_STORAGE_VERSIONS="networking.k8s.io/v1,storage.k8s.io/v1"

As we still write extensions/v1beta1 objects

Are you referring to fact with this PR, by default we still store objects as extensions/v1beta1? I agree. But we should test that we have the ability of converting the storage versions now.

Anyway, it doesn't block this PR.

pkg/apis/extensions/register.go
@@ -63,8 +64,8 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&ReplicaSetList{},
&PodSecurityPolicy{},
&PodSecurityPolicyList{},
&NetworkPolicy{},
&NetworkPolicyList{},
&networking.NetworkPolicy{},
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being dense, why do we need to reigster the networking.NetworkPolicy here?

Copy link
Member

Choose a reason for hiding this comment

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

Could you confirm my understanding of the impact of this PR? I think this PR doesn't affect users at all. Users will still be able to access extensions/v1beta1/networkpolicy and networking.k8s.io/v1/networkpolicy. And the object can be stored in both versions in the etcd, according to user's choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, to the outside nothing changes. We only merge the internal types. We do exactly the same trick with autoscaling and apps (unfortunately still conversely, i.e. internal types are in extensions; we should change that).

About your question: yes we need the internal types being registered under extensions if the external types should work for that group. We never switch to another internal group during conversion. Technically, the types coming out of such a conversion will be the internal types from the networking group.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. It makes sense.

@thockin
Copy link
Member

thockin commented Aug 11, 2017

Today we support both extensions/v1beta1 and networking/v1 for NetworkPolicy. ISTR that we still write to etcd in v1beta1 form. Won't this break that?

@sttts
Copy link
Contributor Author

sttts commented Aug 11, 2017

ISTR that we still write to etcd in v1beta1 form. Won't this break that?

We still write to storage with extensions/v1beta1, yes. The cohabitation is still in place. Changing that (we should!) is worth a follow-up.

@caesarxuchao I will double check that we have tests in place to prove that storage-wise nothing changes (we have a etcd storage tests now due to @enj which actually does end-to-end checks that data in etcd is correct).

@thockin
Copy link
Member

thockin commented Aug 11, 2017

This machinery confuses the crap out of me. If you tell me it works and that stored v1beta1 objects can up-convert to v1, I am happy.

@danwinship The deprecation policy says beta needs one overlap release, which we did, but pragmatically don't we need a second overlap release where we change the default storage-version to v1 ? I.e. we should make that change in 1.8?

@sttts
Copy link
Contributor Author

sttts commented Aug 11, 2017

don't we need a second overlap release where we change the default storage-version to v1

I think so too.

@sttts
Copy link
Contributor Author

sttts commented Aug 11, 2017

Just verified: we store extensions/v1beta1: https://github.com/sttts/kubernetes/blob/85e2e5dd9a21688af0196e715e7494f18cf71b21/test/integration/etcd/etcd_storage_path_test.go#L240

@thockin
Copy link
Member

thockin commented Aug 11, 2017 via email

@sttts
Copy link
Contributor Author

sttts commented Aug 11, 2017 •
edited

The natural implication is that you CAN'T release a GA object and deprecate
a beta object with one overlap, right?

We can

  • deprecate a beta,
  • release the GA,
  • change the storage to GA,
  • publish a release note to migrate

in one release. Obviously, this kills the ability to roll back. I don't see this being mentioned in https://kubernetes.io/docs/reference/deprecation-policy/. Of course, if you want roll-back, your conclusion is right.

On the other hand, we can always keep GroupVersions internally in order to read from storage (if we forgot to tell people to migrate). We don't have to register the GroupVersion as a http handler.

@danwinship
Copy link
Contributor

Obviously, this kills the ability to roll back. I don't see this being mentioned in https://kubernetes.io/docs/reference/deprecation-policy/. Of course, if you want roll-back, your conclusion is right.

Being able to roll back was explicitly raised as a concern in the v1 upgrade PR: #39164 (comment), #39164 (comment). So maybe the deprecation policy should be updated.

@danwinship
Copy link
Contributor

Fixes #46626

@sttts sttts 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 14, 2017
@sttts sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017
@sttts sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 14, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@cmluciano
Copy link

Might need some updates with the BUILD file?

W0814 13:16:51.547] ERROR: /workspace/k8s.io/kubernetes/pkg/registry/extensions/networkpolicy/BUILD:27:1: //pkg/registry/extensions/networkpolicy:go_default_test: missing input file '//pkg/registry/extensions/networkpolicy:doc.go'.
W0814 13:16:51.547] ____Building complete.
W0814 13:16:51.547] ERROR: /workspace/k8s.io/kubernetes/pkg/registry/extensions/networkpolicy/BUILD:27:1 1 input file(s) do not exist.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

5 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@sttts sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 15, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2017
@sttts sttts force-pushed the sttts-unify-network-types branch 2 times, most recently from c645d1f to 6bc9f3f Compare August 15, 2017 09:56
@sttts sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 15, 2017 •
edited

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

Test name Commit Details Rerun command
pull-kubernetes-cross c645d1fd6cf91517ee462f23a9f18ac5832de638 link /test pull-kubernetes-cross

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

@k8s-github-robot k8s-github-robot merged commit 6dc567a into kubernetes:master Aug 15, 2017
@danwinship danwinship mentioned this pull request Sep 5, 2017
Merged
@sttts sttts mentioned this pull request Nov 1, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Nov 15, 2017
Merge pull request #51955 from danwinship/update-networkpolicy-storage
9058769 
Automatic merge from submit-queue (batch tested with PRs 55648, 55274, 54982, 51955, 55639). 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>.

Swap NetworkPolicy storage to networking.k8s.io/v1

Finishes(?) the NetworkPolicy v1 migration.
Fixes #50604

The integration test passes. I copied the test-update-storage-objects.sh change from #50327 and have no idea if it's right.

/cc @sttts @caesarxuchao @thockin

**Release note**:
```release-note
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao caesarxuchao left review comments

Assignees

@caesarxuchao caesarxuchao

@janetkuo janetkuo

@thockin thockin

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@sttts @shyamjvs @thockin @danwinship @k8s-github-robot @fejta-bot @cmluciano @k8s-ci-robot @caesarxuchao @janetkuo