Implement orphaning-deletion commands #1008

roytman · 2019-06-20T05:28:32Z

Fix for #690

marun · 2019-06-26T14:24:27Z

Thank you for doing this! I am reviewing.

xunpan

Thanks for your code. The function is fine to me. Please see my comments below.

By the way, we need to add test code for the related function for CI. It is fine to have a sepearated PR for that.

pkg/kubefedctl/federate/util.go

pkg/kubefedctl/orphaning/enable.go

pkg/kubefedctl/federate/util.go

xunpan · 2019-07-03T01:01:34Z

pkg/kubefedctl/orphaning/disable.go

+
+	orphaning_disable_example = `
+		# Removes the "orphaning enable" annotation from a federated resources of Deployment foo, 
+		kubefedctl orphaning disable deployments.apps foo --host-cluster-context=cluster1`


deployments.apps is type config object name. If my understanding is right, it should be Federated* type. Same comments for enable.go, status.go.

It is an example of the command (flag) usage, so it operates with a specific types. In the above explanation mentions a generic "federated resource"
The same convention exists with others kubefedctl and kubectl commands

I think the command line can operate on a specified federated type with object name specified as documented in user guide.

kubefedctl orphaning-deletion enable <federated type> <name>

My question is about the example of using <federated type>

deployments.apps is a FederatedTypeConfig object but not a federated type.

$ kubectl -n kube-federation-system get federatedtypeconfigs deployments.apps NAME AGE deployments.apps 7m

I think a name with Federated prefixed makes more sense for the example. E.g. kubefedctl orphaning enable FederatedDeployment foo

Got it, thanks for the explanation. Fixed.

pkg/kubefedctl/orphaning/disable.go

xunpan · 2019-07-03T01:13:03Z

pkg/kubefedctl/orphaning/disable.go

+		return nil
+	}
+	st, ok := annotations[sync.OrphanManagedResources]
+	if !ok || st == "false" {


[sugguestion] functionally, it is good. To me, a definite consistent behavior should be better for a command line operation. If enable, add/update annotation to be true. If disable, there should no the annotation no matter it is false or true before the command line is executed.

I wanted to save an addition update, but I'd agree with you, better have a consistent behavior.
Fixed.

pkg/kubefedctl/orphaning/orphaning.go

roytman · 2019-07-03T13:25:46Z

Hi @xunpan , thanks for your review. I fixed part of your comments and added responses to others. Please take a look, if it is OK now.
Thanks Alexey.

xunpan

2 minor readability comments.

And for commits, could you please squash them. It is not encouraged to keep commits that are only used to fix previous commit in one PR.

xunpan · 2019-07-05T03:16:48Z

pkg/kubefedctl/federate/util.go

 	"sigs.k8s.io/kubefed/pkg/apis/core/typeconfig"
 	ctlutil "sigs.k8s.io/kubefed/pkg/controller/util"
 	"sigs.k8s.io/kubefed/pkg/kubefedctl/enable"
-	"sigs.k8s.io/kubefed/pkg/kubefedctl/options"
+	"sigs.k8s.io/kubefed/pkg/kubefedctl/util"
+	"sigs.k8s.io/yaml"


For better readability, it is import convention in kubefed:

import( "core-packages" "3rd-party-packages" "kubefed-packages" )

E.g.:

import ( "io" "os" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/kubefed/pkg/kubefedctl/enable" "sigs.k8s.io/kubefed/pkg/kubefedctl/options" )

Fixed it,
looks like kubefed uses a slightly different import convention:

import( "core-packages" "3rd-party-packages" "K8s packages" "kubefed-packages" ) ``` Is it correct ?

xunpan · 2019-07-05T03:23:27Z

pkg/kubefedctl/orphaning/enable.go

+
+// RunEnable implements the `enable` command.
+func (o *orphanResource) RunEnable(cmdOut io.Writer, config util.FedConfig) error {
+	fedResource, resources, err := o.GetFederatedResource(config)


resources is actually a resourceClient I think. It is also better to have resources in GetFederatedResource as resourceClient. It is a little confuse to use resource and resources in GetFederatedResource for different types of object.

agree, fixed

xunpan · 2019-07-05T05:02:37Z

@roytman
This change impacts user interface. Please also update CHANGELOG.md in Unreleased section.
E.g.
https://github.com/kubernetes-sigs/kubefed/pull/1010/files

roytman · 2019-07-05T07:56:18Z

2 minor readability comments.

And for commits, could you please squash them. It is not encouraged to keep commits that are only used to fix previous commit in one PR.
Did it, (I was under impression that all squashes are done during merge.)

roytman · 2019-07-05T07:56:52Z

This change impacts user interface. Please also update CHANGELOG.md in Unreleased section.
E.g.
https://github.com/kubernetes-sigs/kubefed/pull/1010/files

Done, thanks

xunpan

The code looks fine to me.

The comment is for .gitignore.

xunpan · 2019-07-08T02:35:48Z

.gitignore

+
+# Files generated by JetBrains IDEs, e.g. IntelliJ IDEA
+.idea/
+*.iml


.gitignore is used for project related files maintenance in kubefed.

I finally agreed this rule after thinking it over. I saw some projects which use .gitignore file with hundreds of lines code with editor specific ignore. It is hard to read and maintain. You can refer to #871.

I see the point.
Actually, I copied the entries from the Kubernetes main repository, and probably worth to be inline with it. But this discussion is out of this PR scope.
I removed the entries.

xunpan · 2019-07-09T01:33:08Z

Thanks Roytman!
/lgtm

marun

Nits aside, this is looking good!

Without #828 there isn't an easy path to testing of the cli just yet, but I would appreciate some common code paths being created (in the vein of the managed label helpers) to leverage the coverage for the sync controller check and crudtester check.

pkg/kubefedctl/federate/util.go

pkg/kubefedctl/orphaning/disable.go

pkg/kubefedctl/orphaning/status.go

pkg/kubefedctl/orphaning/orphaning.go

pkg/kubefedctl/orphaning/enable.go

pkg/kubefedctl/orphaning/disable.go

roytman · 2019-07-11T14:27:55Z

@marun , thanks for the review.

Without #828 there isn't an easy path to testing of the cli just yet, but I would appreciate some common code paths being created (in the vein of the managed label helpers) to leverage the coverage for the sync controller check and crudtester check.

I updated the code.

marun

Thank you for cleanup, nice work! A few more nits inline. The only functional change request is to allow the addition or removal of the annotation for resources that don't pass the weak check for a federated resource.

pkg/controller/sync/controller.go

pkg/controller/util/orphaninganotation.go

pkg/kubefedctl/orphaning/disable.go

pkg/kubefedctl/orphaning/orphaning.go

marun · 2019-07-14T20:09:56Z

pkg/kubefedctl/orphaning/orphaning.go

+func (o *orphanResource) GetFederatedResource(resourceClient dynamic.ResourceInterface) (*unstructured.Unstructured, error) {
+	resource, err := resourceClient.Get(o.resourceName, metav1.GetOptions{})
+	if err != nil {
+		return nil, errors.Wrapf(err, "Error retrieving resource %s %q", o.typeName,


Would it be possible to use the kind here rather than the type name the user passed in?

Due to separation of GetResourceClient and GetFederatedResource, it will be complicated. In the GetFederatedResource I don't have a reference to resourceClient only to the dynamic.ResourceInterface.
In order to use Kind, I have to

or return Kind from GetResourceClient and provide it to GetFederatedResource

or move targetClient.Resources(o.resourceNamespace) out of GetResourceClient.

Both options look problematic for me. What do you think ?

I think the first option is a change worth making to ensure the UX consistency.

I implemented the first option.
For generalizing, GetResourceClient returns entire *metav1.APIResource, but it can be easily replace by Kind only.
However, there is an additional option: the current error message looks like
Error: Error retrieving resource FederatedConfigMap "default/game-config1": federatedconfigmaps.types.kubefed.k8s.io "game-config1" not found

Do we really have to print the resource Kind or its user provided type? Anyway the dynamic client prints (at list in the most common case) the resource name and group. Maybe we should just return the dynamic client error.
What do you think?

pkg/kubefedctl/orphaning/orphaning.go

roytman · 2019-07-16T11:12:02Z

The only functional change request is to allow the addition or removal of the annotation for resources that don't pass the weak check for a federated resource.

If a resource doesn't pass the weak check for a federated resource, I suggest to print a warning and continue the processing. What do you think ?

marun

👍 to printing a warning if the weak check passes rather than failing.

Thank you for cleanup! Nothing functional remains. Nits inline, and there are still some outstanding requests for change from my previous review that you may have missed (mainly wording of help text).

pkg/controller/sync/controller.go

pkg/kubefedctl/orphaning/disable.go

marun · 2019-07-16T22:21:35Z

pkg/kubefedctl/orphaning/orphaning.go

+func (o *orphanResource) GetFederatedResource(resourceClient dynamic.ResourceInterface) (*unstructured.Unstructured, error) {
+	resource, err := resourceClient.Get(o.resourceName, metav1.GetOptions{})
+	if err != nil {
+		return nil, errors.Wrapf(err, "Error retrieving resource %s %q", o.typeName,


I think the first option is a change worth making to ensure the UX consistency.

marun · 2019-07-16T22:27:09Z

pkg/kubefedctl/orphaning/orphaning.go

+	}
+	klog.V(2).Infof("API Resource for %s.%s/%s found", apiResource.Name, apiResource.Group, apiResource.Version)
+	if !util.IsFederatedAPIResource(apiResource.Kind, apiResource.Group) {
+		fmt.Fprintf(cmdOut, "Warning: looks like the resource %s.%s/%s is not a Federated object\n",


s/looks like the resource//
s/is not a Federated object/does not appear to be a federated resource/

As to how to compose name and group to avoid a trailing . for core groups, please use the GroupQualifiedName function.

I was not aware about the GroupQualifiedName function, thanks.
I changed the warning message according to your suggestion, however, as I understood the function IsFederatedAPIResource cannot 100% guarantee that the checked resource is not a Federated resource. Therefore, I'd suggest a weaker statement.
What do you think about might not be a federated resource

~~I think does not appear to be is equivalent to might not be (i.e. this does not appear to be correct vs this is not correct).~~

On second thought, please go ahead with your proposed change might not be a federated resource.

roytman · 2019-07-17T08:50:51Z

Thanks for your reviews and sorry for the missed comments.
I have 2 suggestions: one about the warning message, maybe the statement should be weaker, and another about the error message in GetFederatedResource .
Please take a look.

marun

Github isn't letting me reply to your comment about GetFederatedResource. I agree that the message that resulted from my suggestion is unnecessarily verbose, and I apologize for causing you extra work. How about Failed to retrieve resource: %v (no need for kind in that case)?

pkg/kubefedctl/orphaning/status.go

pkg/kubefedctl/orphaning/disable.go

marun · 2019-07-17T19:45:25Z

Nice work, thank you!

/approve
/lgtm

k8s-ci-robot · 2019-07-17T19:45:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun, roytman

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [marun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from danehans and gyliu513 June 20, 2019 05:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 20, 2019

roytman force-pushed the orphaning branch 2 times, most recently from dcd6f8d to 7884655 Compare June 20, 2019 05:55

marun self-requested a review June 26, 2019 14:23

xunpan suggested changes Jul 3, 2019

View reviewed changes

roytman force-pushed the orphaning branch from 41fa575 to 258f283 Compare July 4, 2019 07:09

xunpan reviewed Jul 5, 2019

View reviewed changes

roytman force-pushed the orphaning branch 2 times, most recently from 86d2310 to 1bbd937 Compare July 5, 2019 07:53

roytman force-pushed the orphaning branch from 1bbd937 to cb3da22 Compare July 5, 2019 11:47

xunpan reviewed Jul 8, 2019

View reviewed changes

roytman force-pushed the orphaning branch from cb3da22 to fb64643 Compare July 8, 2019 14:33

k8s-ci-robot assigned xunpan Jul 9, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019

marun suggested changes Jul 11, 2019

View reviewed changes

roytman force-pushed the orphaning branch from fb64643 to 5f3fff0 Compare July 11, 2019 11:43

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2019

roytman force-pushed the orphaning branch 3 times, most recently from edce614 to 9286659 Compare July 11, 2019 13:27

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 11, 2019

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 11, 2019

roytman force-pushed the orphaning branch from 9286659 to 284c6b6 Compare July 11, 2019 13:38

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

roytman force-pushed the orphaning branch from 284c6b6 to b79f678 Compare July 12, 2019 18:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019

roytman force-pushed the orphaning branch from b79f678 to a86b056 Compare July 14, 2019 11:08

marun suggested changes Jul 14, 2019

View reviewed changes

roytman force-pushed the orphaning branch 3 times, most recently from 7dda275 to c8356c3 Compare July 16, 2019 11:37

marun suggested changes Jul 16, 2019

View reviewed changes

roytman force-pushed the orphaning branch from c8356c3 to e185d56 Compare July 17, 2019 08:45

marun suggested changes Jul 17, 2019

View reviewed changes

pkg/kubefedctl/orphaning/status.go Show resolved Hide resolved

pkg/kubefedctl/orphaning/disable.go Outdated Show resolved Hide resolved

Implement orphaning-deletion commands

7faea23

roytman force-pushed the orphaning branch from e185d56 to 7faea23 Compare July 17, 2019 18:35

k8s-ci-robot assigned marun Jul 17, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019

marun approved these changes Jul 17, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2019

k8s-ci-robot merged commit 16493d5 into kubernetes-retired:master Jul 17, 2019

roytman deleted the orphaning branch July 18, 2019 21:43

marun mentioned this pull request Jul 19, 2019

Facilitate adding the annotation on federated object to specify users intent of orphaning/not orphaning target/managed resources. #690

Closed

Implement orphaning-deletion commands #1008

Implement orphaning-deletion commands #1008

Conversation

roytman commented Jun 20, 2019

marun commented Jun 26, 2019

xunpan left a 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

roytman commented Jul 3, 2019

xunpan left a 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

xunpan commented Jul 5, 2019

roytman commented Jul 5, 2019

roytman commented Jul 5, 2019 • edited Loading

xunpan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunpan commented Jul 9, 2019

marun left a comment

Choose a reason for hiding this comment

roytman commented Jul 11, 2019

marun left a 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

roytman commented Jul 16, 2019

marun left a 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

marun Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roytman commented Jul 17, 2019

marun left a comment

Choose a reason for hiding this comment

marun commented Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

roytman commented Jul 5, 2019 •

edited

Loading

marun Jul 17, 2019 •

edited

Loading