Drop extensions internal Network* types #50327

sttts · 2017-08-08T16:20:11Z

sttts · 2017-08-08T16:20:28Z

pkg/apis/extensions/v1beta1/zz_generated.conversion.go

@@ -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 {


this looks wrong

And goes away after make clean.

sttts · 2017-08-09T14:31:04Z

/retest

sttts · 2017-08-09T16:23:14Z

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

shyamjvs · 2017-08-09T16:31:36Z

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

sttts · 2017-08-10T09:35:53Z

/assign @thockin

@thockin is this approved?

caesarxuchao · 2017-08-10T21:19:54Z

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"


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.

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

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.

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

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

caesarxuchao · 2017-08-10T21:22:07Z

pkg/apis/extensions/register.go

@@ -63,8 +64,8 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&ReplicaSetList{},
 		&PodSecurityPolicy{},
 		&PodSecurityPolicyList{},
-		&NetworkPolicy{},
-		&NetworkPolicyList{},
+		&networking.NetworkPolicy{},


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

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.

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.

Thanks for the explanation. It makes sense.

thockin · 2017-08-11T06:18:11Z

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 · 2017-08-11T06:34:22Z

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 · 2017-08-11T06:37:23Z

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 · 2017-08-11T06:48:17Z

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

I think so too.

sttts · 2017-08-11T06:55:09Z

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

thockin · 2017-08-11T06:55:32Z

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

…

On Thu, Aug 10, 2017 at 11:48 PM, Dr. Stefan Schimanski < ***@***.***> wrote: don't we need a second overlap release where we change the default storage-version to v1 I think so too. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#50327 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVBD6sgYn4shqBYeC7Iqmjifg4f9Zks5sW_lFgaJpZM4Ow-H5> .

sttts · 2017-08-11T07:19:32Z

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 · 2017-08-11T14:01:38Z

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 · 2017-08-11T14:16:56Z

Fixes #46626

fejta-bot · 2017-08-14T12:50:27Z