Mega-rename of service(s)/controller(s)/podController(s) to workload(s) #1777

2opremio · 2019-02-28T02:23:31Z

There was a mixture of the term service and controller throughout the whole codebase meaning the same thing. It was confusing for the newcomer and unpleasant to read.

I don't know if the term service was supposed to be an abstraction agnostic to Kubernetes for things which can be updated in a cluster. If so, it was leaking considerably, including fluxctl and data structure names.

I left "service(s)" strings/identifiers as is were unavoidable, to preserve backwards compatibility (e.g. query parameters, serialized data structure fields exposed to the http client etc ...).

squaremo · 2019-02-28T08:30:17Z

Apparently even "controller" is wrong -- it refers to a program that interprets resources and e.g., starts pods. We eventually started naming the Deployments etc. "workloads", though not very visibly.

api/v6/api.go

2opremio · 2019-02-28T09:30:42Z

Apparently even "controller" is wrong -- it refers to a program that interprets resources and e.g., starts pods. We eventually started naming the Deployments etc. "workloads", though not very visibly.

I have seen that, but I (wrongly) assumed it meant something else. I am happy to rename everything to workload if that works better (even in fluxctl, deprecating the controller argument).

Let me know if that would make this PR acceptable.

squaremo · 2019-02-28T10:27:48Z

I am happy to rename everything to workload if that works better

A mass rename of Service and Controller to Workload would be mostly correct, I think. Maybe we should start a glossary.

I'll go through all the actual changes and see whether each one makes sense.

2opremio · 2019-02-28T11:54:39Z

Upon further thinking I am not sure I like Workload either. It's a familiar term, though not very precise and it doesn't go well with the fact that we explicitly exclude pods which is counterintuitive (although ... who uses them in isolation anyways).

Controller is more specific (although maybe too much since it's a k8s term, but I don't see it as a huge problem at this point). I guess the reason for moving towards Workload is that we now support HelmReleases which aren't controllers?

I don't have a better suggestion at this point but I think it's worth spending some time thinking about it. I don't want to go through another mega-rename anytime soon (particularly since I plan to update fluxctl's flags accordingly).

@hiddeco @stefanprodan Thoughts?

hiddeco · 2019-02-28T12:01:18Z

Controller is more specific (although maybe too much since it's a k8s term, but I don't see it as a huge problem at this point).

If I read the docs correctly the term Controller is a ReplicaSet in Kubernetes. All other type of objects are Workloads.

Given that with fluxctl we are able to list various types of objects (with an exclusion for Pods), I would say the term Workload is correct.

stefanprodan · 2019-02-28T12:01:44Z

I vote for workload, is what we use in Weave Cloud. Also Istio and other projects are using workload to define a group of pods owned by a deployment, statefulset or daemonset

squaremo · 2019-02-28T12:08:32Z

Controller is more specific (although maybe too much since it's a k8s term, but I don't see it as a huge problem at this point).

I have had Kubernetes-embedded people express confusion over the use of "controller" to refer to Deployments, etc., rather than the programs that interpret them. The consensus terminology seems to be that "workload" means a definition of something for the cluster to run, and a "controller" is a particular kind of program.

squaremo · 2019-02-28T12:10:58Z

.. though I should also note that within Kubernetes docs and code and so on, it's not especially consistent and has changed over time, too.

2opremio · 2019-02-28T12:36:25Z

Workload it is then

2opremio · 2019-02-28T12:36:58Z

Thanks a lot for all the comments! Again, I'm really enjoying working with you guys!

2opremio · 2019-03-01T12:47:31Z

Workload it is then

It took forever but it's done. I think it's going to take me some time to gain enough courage before doing this sort of thing again :)

I also removed fluxctl's --service flags and list-services command (they were just printing an error anyways). Please let me know if it is too early and I will revert it.

hiddeco

❤️

stefanprodan

🥇

daemon/daemon_test.go

2opremio · 2019-03-04T11:48:48Z

I rebased and addressed the comments

squaremo

Some completely understandable typos, and two main requests for changes:

I think changing the RPC methods will break compatibility in a way that's not intended, so can we leave those for now;
Changing field names in things that get serialised, even with compensating tags, can make troubleshooting problems more difficult; so can we leave those for now, and plan to move serialised things into versioned packages (to be phased out eventually)

EDIT: Fons reverted the field name changes.

cluster/cluster.go

squaremo · 2019-03-04T11:15:29Z

cmd/fluxctl/list_images_cmd.go

@@ -15,42 +15,39 @@ import (
 	"github.com/weaveworks/flux/update"
 )

-type controllerShowOpts struct {
+type imageListOpts struct {


Oh good inclusion, thanks for changing this too

cmd/fluxctl/list_workloads_cmd.go

cmd/fluxctl/policy_cmd.go

cmd/fluxctl/release_cmd.go

cmd/fluxctl/root_cmd.go

cmd/fluxctl/unlock_cmd.go

remote/rpc/clientV11.go

squaremo · 2019-03-04T12:32:10Z

update/automated.go

-	ServiceID flux.ResourceID
-	Container resource.Container
-	ImageID   image.Ref
+	WorkloadID flux.ResourceID


(NB to myself: this looks like it might be used in the API, but isn't)

update/release_image.go

2opremio · 2019-03-04T13:38:30Z

I think changing the RPC methods will break compatibility in a way that's not intended, so can we leave those for now;

I think this will lead to reverting a big part of the renaming. Do you mean the method names or also the type names? (I don't know the details of net/rpc and how arguments are passed)

squaremo · 2019-03-05T16:08:19Z

Owps, looks like we angered pflag:

$ fluxctl --help
panic: unlock flag redefined: controller

goroutine 1 [running]:
github.com/weaveworks/flux/vendor/github.com/spf13/pflag.(*FlagSet).AddFlag(0xc0003fa200, 0xc000214d20)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/vendor/github.com/spf13/pflag/flag.go:836 +0x870
github.com/weaveworks/flux/vendor/github.com/spf13/pflag.(*FlagSet).VarPF(0xc0003fa200, 0x1466c60, 0xc0002b87d0, 0x12b3d85, 0xa, 0x12af52f, 0x1, 0x12bd141, 0x14, 0xc000214be0)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/vendor/github.com/spf13/pflag/flag.go:819 +0x10b
github.com/weaveworks/flux/vendor/github.com/spf13/pflag.(*FlagSet).VarP(0xc0003fa200, 0x1466c60, 0xc0002b87d0, 0x12b3d85, 0xa, 0x12af52f, 0x1, 0x12bd141, 0x14)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/vendor/github.com/spf13/pflag/flag.go:825 +0x8e
github.com/weaveworks/flux/vendor/github.com/spf13/pflag.(*FlagSet).StringVarP(0xc0003fa200, 0xc0002b87d0, 0x12b3d85, 0xa, 0x12af52f, 0x1, 0x0, 0x0, 0x12bd141, 0x14)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/vendor/github.com/spf13/pflag/string.go:42 +0xaa
main.(*workloadUnlockOpts).Command(0xc0002b8780, 0xc0002b8780)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/unlock_cmd.go:39 +0x2d1
main.(*rootOpts).Command(0xc0000f1860, 0xc0000f1860)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/root_cmd.go:87 +0x6d8
main.run(0xc00000c090, 0x1, 0x1, 0xc0000b4058)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/main.go:12 +0x4d
main.main()
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/main.go:29 +0x62

squaremo · 2019-03-05T16:35:26Z

looks like we angered pflag:

It's just a near-duplicate line in cmd/fluxctl/unlock_cmd.go

Partly for historical reasons, there was a mixture of the term _service_, _controller_ and _workload_ throughout the whole codebase meaning the same thing. It was confusing for the newcomer and unpleasant to read. I left "service(s)" strings/identifiers as is were unavoidable to preserve backwards compatibility (e.g. URL pathas and query parameters, serialized data structure fields exposed to the http client etc ...). I renamed all the _controller_ commands and flags in `fluxctl` to _workload_ equivalents, preserving backwards compatibility (deprecating `--controller`).

They were deprecated and leading to an error when used.

This is to avoid confusion between names serialized data and names used in the code. These field names will be renamed again when versioning is in-place for the structs.

2opremio · 2019-03-05T17:44:24Z

Owps, looks like we angered pflag:

Sigh, sorry about this. I did test fluxctl at some point, but obviously not recently enough.

It's fixed now.

Leftover from #1777

2opremio requested review from squaremo and hiddeco February 28, 2019 02:23

2opremio force-pushed the rename-services-controllers branch 4 times, most recently from 0d5cc37 to 6d975d4 Compare February 28, 2019 02:36

squaremo reviewed Feb 28, 2019

View reviewed changes

api/v6/api.go Outdated Show resolved Hide resolved

2opremio force-pushed the rename-services-controllers branch from 6d975d4 to 84beebb Compare March 1, 2019 12:43

2opremio changed the title ~~Rename service(s) to controller(s)~~ Mega-rename of service(s)/controller(s)/podController(s) to workload(s) Mar 1, 2019

2opremio requested a review from stefanprodan March 1, 2019 12:48

hiddeco approved these changes Mar 1, 2019

View reviewed changes

stefanprodan approved these changes Mar 1, 2019

View reviewed changes

2opremio commented Mar 4, 2019

View reviewed changes

daemon/daemon_test.go Outdated Show resolved Hide resolved

2opremio commented Mar 4, 2019

View reviewed changes

daemon/daemon_test.go Outdated Show resolved Hide resolved

2opremio force-pushed the rename-services-controllers branch from 84beebb to e067647 Compare March 4, 2019 11:48

squaremo suggested changes Mar 4, 2019

View reviewed changes

2opremio force-pushed the rename-services-controllers branch 2 times, most recently from 2799b66 to 5fb9daf Compare March 4, 2019 13:07

2opremio force-pushed the rename-services-controllers branch 3 times, most recently from 207ea2e to 8f6ab6a Compare March 4, 2019 15:33

2opremio and others added 5 commits March 5, 2019 18:43

Remove fluxctl's --service flags and list-services command

1ebb19f

They were deprecated and leading to an error when used.

Revert renaming of serialized struct fields

51e5e0f

This is to avoid confusion between names serialized data and names used in the code. These field names will be renamed again when versioning is in-place for the structs.

Remove unused errors

5ea5a71

Revert all the renamings breaking the API

92a3aac

2opremio force-pushed the rename-services-controllers branch from 8f6ab6a to 92a3aac Compare March 5, 2019 17:43

squaremo approved these changes Mar 13, 2019

View reviewed changes

2opremio merged commit 546ea71 into fluxcd:master Mar 13, 2019

2opremio deleted the rename-services-controllers branch March 13, 2019 11:59

2opremio mentioned this pull request Apr 11, 2019

Release Flux v1.12.0 #1921

Merged

2opremio added a commit that referenced this pull request May 14, 2019

Rename controller to workload in fluxctl release

1d7023e

Leftover from #1777

2opremio mentioned this pull request May 14, 2019

Rename controller to workload in fluxctl release #2048

Merged

2opremio added a commit that referenced this pull request May 22, 2019

Rename controller to workload in fluxctl release

1f8a990

Leftover from #1777

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mega-rename of service(s)/controller(s)/podController(s) to workload(s) #1777

Mega-rename of service(s)/controller(s)/podController(s) to workload(s) #1777

2opremio commented Feb 28, 2019 •

edited

Loading

squaremo commented Feb 28, 2019

2opremio commented Feb 28, 2019 •

edited

Loading

squaremo commented Feb 28, 2019

2opremio commented Feb 28, 2019

hiddeco commented Feb 28, 2019

stefanprodan commented Feb 28, 2019 •

edited

Loading

squaremo commented Feb 28, 2019

squaremo commented Feb 28, 2019

2opremio commented Feb 28, 2019

2opremio commented Feb 28, 2019

2opremio commented Mar 1, 2019 •

edited

Loading

hiddeco left a comment

stefanprodan left a comment

2opremio commented Mar 4, 2019

squaremo left a comment •

edited

Loading

squaremo Mar 4, 2019

squaremo Mar 4, 2019

2opremio commented Mar 4, 2019

squaremo commented Mar 5, 2019

squaremo commented Mar 5, 2019

2opremio commented Mar 5, 2019 •

edited

Loading

Mega-rename of service(s)/controller(s)/podController(s) to workload(s) #1777

Mega-rename of service(s)/controller(s)/podController(s) to workload(s) #1777

Conversation

2opremio commented Feb 28, 2019 • edited Loading

squaremo commented Feb 28, 2019

2opremio commented Feb 28, 2019 • edited Loading

squaremo commented Feb 28, 2019

2opremio commented Feb 28, 2019

hiddeco commented Feb 28, 2019

stefanprodan commented Feb 28, 2019 • edited Loading

squaremo commented Feb 28, 2019

squaremo commented Feb 28, 2019

2opremio commented Feb 28, 2019

2opremio commented Feb 28, 2019

2opremio commented Mar 1, 2019 • edited Loading

hiddeco left a comment

Choose a reason for hiding this comment

stefanprodan left a comment

Choose a reason for hiding this comment

2opremio commented Mar 4, 2019

squaremo left a comment • edited Loading

Choose a reason for hiding this comment

squaremo Mar 4, 2019

Choose a reason for hiding this comment

squaremo Mar 4, 2019

Choose a reason for hiding this comment

2opremio commented Mar 4, 2019

squaremo commented Mar 5, 2019

squaremo commented Mar 5, 2019

2opremio commented Mar 5, 2019 • edited Loading

2opremio commented Feb 28, 2019 •

edited

Loading

2opremio commented Feb 28, 2019 •

edited

Loading

stefanprodan commented Feb 28, 2019 •

edited

Loading

2opremio commented Mar 1, 2019 •

edited

Loading

squaremo left a comment •

edited

Loading

2opremio commented Mar 5, 2019 •

edited

Loading