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.

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

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

Merged
merged 5 commits into from Mar 13, 2019

Conversation

@2opremio
Copy link
Collaborator

commented Feb 28, 2019

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

@2opremio 2opremio requested review from squaremo and hiddeco Feb 28, 2019
@2opremio 2opremio force-pushed the 2opremio:rename-services-controllers branch 4 times, most recently from 0d5cc37 to 6d975d4 Feb 28, 2019
@squaremo

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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 Outdated Show resolved Hide resolved
@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2019

Workload it is then

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2019

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

@2opremio 2opremio force-pushed the 2opremio:rename-services-controllers branch from 6d975d4 to 84beebb Mar 1, 2019
@2opremio 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

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

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.

@2opremio 2opremio requested a review from stefanprodan Mar 1, 2019
@hiddeco
hiddeco approved these changes Mar 1, 2019
Copy link
Member

left a comment

❤️

Copy link
Member

left a comment

🥇

daemon/daemon_test.go Outdated Show resolved Hide resolved
daemon/daemon_test.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the 2opremio:rename-services-controllers branch from 84beebb to e067647 Mar 4, 2019
@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

I rebased and addressed the comments

Copy link
Member

left a comment

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 Outdated Show resolved Hide resolved
@@ -15,42 +15,39 @@ import (
"github.com/weaveworks/flux/update"
)

type controllerShowOpts struct {
type imageListOpts struct {

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 4, 2019

Member

Oh good inclusion, thanks for changing this too

cmd/fluxctl/list_workloads_cmd.go Outdated Show resolved Hide resolved
cmd/fluxctl/policy_cmd.go Outdated Show resolved Hide resolved
cmd/fluxctl/release_cmd.go Outdated Show resolved Hide resolved
cmd/fluxctl/root_cmd.go Outdated Show resolved Hide resolved
cmd/fluxctl/unlock_cmd.go Outdated Show resolved Hide resolved
remote/rpc/clientV11.go Outdated Show resolved Hide resolved
ServiceID flux.ResourceID
Container resource.Container
ImageID image.Ref
WorkloadID flux.ResourceID

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 4, 2019

Member

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

update/release_image.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the 2opremio:rename-services-controllers branch 2 times, most recently from 2799b66 to 5fb9daf Mar 4, 2019
@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

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)

@2opremio 2opremio force-pushed the 2opremio:rename-services-controllers branch 3 times, most recently from 207ea2e to 8f6ab6a Mar 4, 2019
@squaremo

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

looks like we angered pflag:

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

2opremio added 5 commits Feb 28, 2019
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 2opremio force-pushed the 2opremio:rename-services-controllers branch from 8f6ab6a to 92a3aac Mar 5, 2019
@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2019

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.

@2opremio 2opremio merged commit 546ea71 into fluxcd:master Mar 13, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@2opremio 2opremio deleted the 2opremio:rename-services-controllers branch Mar 13, 2019
@2opremio 2opremio referenced this pull request Apr 11, 2019
2opremio added a commit that referenced this pull request May 14, 2019
Leftover from #1777
2opremio added a commit that referenced this pull request May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.