Use Apps api over extensions #2611

rootfs · 2019-02-04T19:40:19Z

Description of your changes:
fix #2601

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

Documentation has been updated, if necessary.
Pending release notes updated with breaking and/or notable changes, if necessary.
Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
Code generation (make codegen) has been run to update object specifications, if necessary.
Comments have been added or updated based on the standards set in CONTRIBUTING.md

rootfs · 2019-02-04T21:01:57Z

failed with the following error

failed to run operator. Error starting agent daemonset: error starting agent daemonset: failed to create rook-ceph-agent daemon set. DaemonSet.apps "rook-ceph-agent" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app":"rook-ceph-agent"}: `selector` does not match template `labels`

travisn · 2019-02-05T15:10:31Z

tests/framework/installer/ceph_manifests_v0.9.go

@@ -215,7 +215,7 @@ rules:
  - update
  - delete
 - apiGroups:
-  - extensions
+  - apps


For the upgrade test, we'll need to revert any changes to this file

I have a change coming in #2522 (hopefully merged today) that will update the rbac needed for the upgrade test. We'll need to do something similar here. This helps us catch what needs to be upgraded by users for the next release.

travisn · 2019-02-05T15:13:54Z

pkg/operator/discover/discover.go

+		Spec: apps.DaemonSetSpec{
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"app": discoverDaemonsetName,


why do we need the selector for the daemonset now?

selector is required to match labels in apps resources
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/apps/validation/validation.go

we still need to update selectors in deployments so CI can pass

BlaineEXE · 2019-02-06T19:05:05Z

Like Travis said, I'd like to see what the upgrade procedure is to go from 0.9.x to master with these changes, but other than that I think it looks good.

galexrt · 2019-02-07T15:43:27Z

@rootfs Why are we going to use apps/v1beta2 instead of apps/v1?
Rook currently supports K8S >= 1.10 (https://rook.io/docs/rook/master/k8s-pre-reqs.html#minimum-version) meaning that apps/v1 can be used instead of the old beta api.

rootfs · 2019-02-07T15:44:20Z

cc @yanniszark for cassandra changes @dyusupov for edgefs changes

rootfs · 2019-02-07T15:54:57Z

@rootfs Why are we going to use apps/v1beta2 instead of apps/v1?
Rook currently supports K8S >= 1.10 (https://rook.io/docs/rook/master/k8s-pre-reqs.html#minimum-version) meaning that apps/v1 can be used instead of the old beta api.

I don't have a problem supporting v1 api

Documentation/edgefs-cluster-crd.md

rootfs · 2019-02-08T14:19:57Z

looks there was a test flake on aws 1.12, other instances went well

rootfs · 2019-02-08T14:20:34Z

@travisn @galexrt any more input?

travisn

@rootfs #2522 was finally merged last night. Could you rebase on that and then let's discuss the upgrade test. thanks

travisn · 2019-02-08T14:59:57Z

tests/framework/installer/ceph_manifests_v0.9.go

@@ -403,7 +403,7 @@ subjects:
  name: rook-ceph-system
  namespace: ` + namespace + `
 ---
-apiVersion: apps/v1beta1
+apiVersion: extensions/v1beta1


please revert this change to ceph_manifests_v0.9.go

galexrt · 2019-02-09T12:58:44Z

Gopkg.lock

@@ -1403,10 +1403,8 @@
    "github.com/yanniszark/go-nodetool/nodetool",
    "k8s.io/api/apps/v1",
    "k8s.io/api/apps/v1beta1",


I'm wondering why this dependency is still here after switching to apps/v1.
Any idea why it is still a dependency? Is it used by "old" or somewhere in test code?

yanniszark · 2019-02-13T18:20:12Z

Checking the PR head for files containing "/extensions/v1beta" returned only files from the vendor folder, so everything seems to have been updated.

$ grep -r -l "/extensions/v1beta" .

./Gopkg.lock
./vendor/k8s.io/kubernetes/pkg/apis/core/v1/BUILD
./vendor/k8s.io/kubernetes/pkg/apis/extensions/
...
./vendor/k8s.io/apiserver/pkg/endpoints/discovery/version.go
./vendor/github.com/rook/operator-kit/Gopkg.lock

@galexrt as for why vendor still contains /extensions/v1beta1, I believe it's because it is a transitive dependency of a library we use, most likely client-go (Godeps file).

rootfs · 2019-02-13T19:24:36Z

cc @sabbot @dyusupov for edgefs operator changes.

pkg/operator/ceph/cluster/mgr/spec.go

travisn · 2019-02-13T20:52:05Z

cluster/examples/kubernetes/ceph/upgrade-from-v0.8-replace.yaml

@@ -41,7 +41,7 @@ rules:
  - update
  - delete
 - apiGroups:
-  - extensions
+  - apps


This is only used for upgrades from 0.8 to 0.9, so we'll need to revert this. We'll need something similar for the v0.9 to v1.0 upgrade before we release though

travisn · 2019-02-13T20:54:39Z

cluster/examples/kubernetes/ceph/operator.yaml

@@ -204,7 +204,7 @@ rules:
  - update
  - delete
 - apiGroups:
-  - extensions
+  - apps


if there are changes to the rbac i would have expected the upgrade_test.go to need an update, otherwise the rbac will not be updated in the test. See here for an example of updating the rbac for a similar change. The upgrade test is what helps us put the final upgrade documentation ready when it's time for release.

travisn · 2019-02-14T15:37:36Z

tests/integration/upgrade_test.go

@@ -155,6 +155,9 @@ func (s *UpgradeSuite) updateClusterRoles() error {
 	if _, err := s.k8sh.DeleteResource("ClusterRole", "rook-ceph-global"); err != nil {
 		return err
 	}
+	if _, err := s.k8sh.DeleteResource("ClusterRole", "rook-ceph-cluster-mgmt"); err != nil {


Do we also need to add the rook-ceph-system role? Looks like there is a change there as well.

Signed-off-by: Huamin Chen <hchen@redhat.com>

rootfs force-pushed the apps-api branch from 77d4fa2 to a7ecd07 Compare February 4, 2019 19:42

rootfs requested review from travisn, galexrt and BlaineEXE February 4, 2019 19:43

travisn requested changes Feb 5, 2019

View reviewed changes

rootfs force-pushed the apps-api branch from 6b22e9d to e935250 Compare February 7, 2019 15:42

galexrt requested changes Feb 7, 2019

View reviewed changes

Documentation/edgefs-cluster-crd.md Outdated Show resolved Hide resolved

rootfs force-pushed the apps-api branch 2 times, most recently from fa2aae2 to 7cbd6b1 Compare February 7, 2019 20:17

travisn reviewed Feb 8, 2019

View reviewed changes

rootfs force-pushed the apps-api branch 2 times, most recently from 0545a07 to 582b493 Compare February 8, 2019 16:26

galexrt reviewed Feb 9, 2019

View reviewed changes

rootfs force-pushed the apps-api branch from 582b493 to 670ada9 Compare February 13, 2019 15:51

rootfs force-pushed the apps-api branch from 8c30ba4 to dc82c65 Compare February 13, 2019 19:22

galexrt requested changes Feb 13, 2019

View reviewed changes

pkg/operator/ceph/cluster/mgr/spec.go Outdated Show resolved Hide resolved

travisn reviewed Feb 13, 2019

View reviewed changes

travisn reviewed Feb 14, 2019

View reviewed changes

rootfs force-pushed the apps-api branch from ce9dfbb to cd54434 Compare February 14, 2019 19:12

convert extensions to apps

1b21e42

Signed-off-by: Huamin Chen <hchen@redhat.com>

rootfs force-pushed the apps-api branch from cd54434 to 1b21e42 Compare February 14, 2019 21:06

travisn approved these changes Feb 14, 2019

View reviewed changes

galexrt approved these changes Feb 14, 2019

View reviewed changes

travisn merged commit 913f0e0 into rook:master Feb 14, 2019

This was referenced Feb 14, 2019

Convert remaining extensions to apps api #2661

Merged

cannot create resource "daemonsets" in API group "apps" in the namespace "rook-ceph-system" #2672

Closed

Update cluster/examples (Cluster)Roles apiGroups #2666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Apps api over extensions #2611

Use Apps api over extensions #2611

rootfs commented Feb 4, 2019

rootfs commented Feb 4, 2019

travisn Feb 5, 2019

travisn Feb 5, 2019

rootfs Feb 7, 2019

travisn Feb 5, 2019

rootfs Feb 5, 2019 •

edited

rootfs Feb 5, 2019

BlaineEXE commented Feb 6, 2019

galexrt commented Feb 7, 2019

rootfs commented Feb 7, 2019

rootfs commented Feb 7, 2019

rootfs commented Feb 8, 2019

rootfs commented Feb 8, 2019

travisn left a comment

travisn Feb 8, 2019

galexrt Feb 9, 2019

yanniszark commented Feb 13, 2019

rootfs commented Feb 13, 2019

travisn Feb 13, 2019

travisn Feb 13, 2019

rootfs Feb 14, 2019

travisn Feb 14, 2019

rootfs Feb 14, 2019

Use Apps api over extensions #2611

Use Apps api over extensions #2611

Conversation

rootfs commented Feb 4, 2019

rootfs commented Feb 4, 2019

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

rootfs Feb 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Feb 6, 2019

galexrt commented Feb 7, 2019

rootfs commented Feb 7, 2019

rootfs commented Feb 7, 2019

rootfs commented Feb 8, 2019

rootfs commented Feb 8, 2019

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanniszark commented Feb 13, 2019

rootfs commented Feb 13, 2019

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

rootfs Feb 5, 2019 •

edited