Add: new routes for daemonsets

[Seb-Solon]

Hi,

I started to implement #509.

I tried to keep it simple : the code is very similar to the replication controller one. Maybe we could refactor it but :

• I'm not that good in Go.
• Types are different in functions parameters, we may need to find a workaround (add an interface and cast object?)

Feel free to comment!

[Seb-Solon]

I'm trying to run test locally to make Travis green. If you have any hint I'll take it :)

I did not manage to get a go test command working as test and code are in separated folder.

[maciaszczykm]

@Seb-Solon To run tests use gulp check command in your console. It will execute all needed tests locally.

[codecov-io]

Current coverage is 95.20%

Merging #510 into master will increase coverage by 0.44%

1. File ...ereader_directive.js (not in diff) was modified. more
   • Misses -6
   • Partials 0
   • Hits +6

@@             master       #510   diff @@
==========================================
  Files           173        173          
  Lines          1332       1332          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1262       1268     +6   
+ Misses           70         64     -6   
  Partials          0          0          

Powered by Codecov. Last updated by e755d63...70f1a1f

[Seb-Solon]

PR is ready IMO. I won't touch it anymore, have fun 😄

[bryk]

PR is ready IMO. I won't touch it anymore, have fun 😄

Hey @Seb-Solon, thanks for doing this! :) Are you saying that you arent going to work on this PR to have it merged?

[Seb-Solon]

No what I meant was : "The CI is green, it's not work in progress anymore". I'm waiting for your comment / review :)

[bryk]

All right, sounds perfect.

Would you mind waiting a few more days with this PR? The problem is on our side: we do not know how to scale the UX of the UI to accommodate more types of resource lists. This is a critical problem that we will be solving in a few days. Once we know how to add new resources to the UI, it will be very easy for us to the such PRs through.

Is this okay for you?

[Seb-Solon]

No problem, take your time :)

[bryk]

Thank you!

[Seb-Solon]

Hi,

Any news about this? I can see it start drifting a bit : 0d6e5bc and 6c3d4bd
I can rebase on v1.0.1 and apply the same diff if you think this PR is still legit.

[bryk]

Wait a few more days to see what's happening with design that we have for handling multiple resources: #589

If it gets accepted, then we can go ahead with implementation

[Seb-Solon]

Hi,

What about this PR ? I saw that you commited things about the new dashboard design.

[bryk]

Oh, yeah. We can merge this PR now :) I'm very happy to work with you to get it merged and have support for DaemonSets in the UI.

[bryk]

We've made lots of changes to backend code since start of this PR. Can you rebase it?

[Seb-Solon]

Rebased, waiting for travis to be green

[floreks]

It appears you need to apply some fixes. We've completely refactored go codebase and now it's divided into packages. First step would be to check our new pattern and try to follow it. Create deamonset package under resource and add there go file with logic. In case of any issues feel free to ask :) We'll support you.

Thanks for help!

[Seb-Solon]

Ok, I'm working on this. Can you give me a command to build the dashboard locally? I can see those . import and this makes fail my previous command I had to build

[bryk]

Hm... If your repo clone is really old, best create and then

git checkout -b titilambert-Add_ds_routes master
git pull https://github.com/titilambert/dashboard.git Add_ds_routes

With this you should have a clean environment.

If this does not help, try entering the root dir of the repo and:

rm -rf node_modules/
npm install
gulp clean
gulp build
gulp backend # to compile backend
gulp test # to test all
gulp backend-test # to test backend only

[Seb-Solon]

Ok I'm done, Travis is green waiting for you input.

[bryk]

@floreks Can you review this also?

[bryk]

Review status: 0 of 17 files reviewed at latest revision, 11 unresolved discussions.

---

src/app/backend/handler/apihandler.go, line 28 [r7] (raw file):

  . "github.com/kubernetes/dashboard/resource/container"
  "github.com/kubernetes/dashboard/resource/deployment"
  . "github.com/kubernetes/dashboard/resource/daemonset"

Do not use dot-imports for new code. The dot imports that are here are for legacy reasons, we plan to remove them ASAP.

Remove the dot and prefix all stuff you use from the package there with "daemonset".

---

src/app/backend/resource/common/podinfo.go, line 106 [r7] (raw file):

// Returns true when a Service with the given selector targets the same Pods (or subset) that
// a Daemon Set with the given selector.
func IsLabelSelectorMatchingforDS(labelSelector map[string]string,

Hmm... This needs a name and documentation that does not mention daemon sets.

---

src/app/backend/resource/common/podinfo.go, line 106 [r7] (raw file):

// Returns true when a Service with the given selector targets the same Pods (or subset) that
// a Daemon Set with the given selector.
func IsLabelSelectorMatchingforDS(labelSelector map[string]string,

Also, I think the argument semantics are flipped. I.e., "testedObjectLabels" is actually label selector and vice versa.

---

src/app/backend/resource/common/resourcechannels.go, line 253 [r7] (raw file):

}

// Returns a pair of channels to a ReplicaSet list and errors that both must be read

s/ReplicaSet/DaemonSet

---

src/app/backend/resource/daemonset/daemonsetcommon.go, line 33 [r7] (raw file):

// DaemonSetPodInfo represents aggregate information about deamon set pods.
type DaemonSetPodInfo struct {

Remove this struct and just use PodInfo from common/podinfo.go. It does the same.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 20 [r7] (raw file):

  "log"

  . "github.com/kubernetes/dashboard/client"

Do not use dot-imports for new code :)

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 50 [r7] (raw file):

  // Aggregate information about pods of this daemon set.
  PodInfo DaemonSetPodInfo `json:"podInfo"`

Use PodInfo from common :)

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 224 [r7] (raw file):

// Returns detailed information about service from given service
func getServiceDetailforDS(service api.Service, daemonSet extensions.DaemonSet,

Can you delete the internal/external endpoints code from this PR? We plan to have a common way of handling this across our codebase, so duplicating work is not necessary.

---

src/app/backend/resource/daemonset/daemonsetlist.go, line 23 [r7] (raw file):

  . "github.com/kubernetes/dashboard/resource/replicationcontroller"
  // TODO(maciaszczykm): Avoid using dot-imports.
  . "github.com/kubernetes/dashboard/resource/event"

Dot imports :)

---

src/app/backend/resource/daemonset/daemonsetpods.go, line 15 [r7] (raw file):

// limitations under the License.

package daemonset

Can you remove this part from the PR? We plan to have better and easier way of getting pods for a controller. It'd also make the PR smaller.

---

src/app/backend/resource/daemonset/daemonsetpodsmetrics.go, line 68 [r7] (raw file):

// Create response structure for API call.
func createResponseforDS(cpuMetrics []heapster.MetricResult, memMetrics []heapster.MetricResult,

This looks like a copy&paste from rcpodsmetrics. Can you reuse code from there?

---

Comments from Reviewable

[bryk]

Nice work! We'll have a few iterations of comments and, hopefully, merge quickly :)

Previously, bryk (Piotr Bryk) wrote…

@floreks Can you review this also?

---

Review status: 0 of 17 files reviewed at latest revision, 11 unresolved discussions.

---

Comments from Reviewable

[floreks]

Review status: 0 of 17 files reviewed at latest revision, 13 unresolved discussions.

---

src/app/backend/handler/apihandler.go, line 191 [r7] (raw file):

  daemonSetWs := new(restful.WebService)
  daemonSetWs.Filter(wsLogger)
  daemonSetWs.Path("/api/v1/daemonsets").

@bryk should we follow already new api concept? api/v1/deamonset/...?

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 63 [r7] (raw file):

// Detailed information about a Pod that belongs to a Daemon Set
type DaemonSetPod struct {

There is no deamon set specific data in this structure. Could we just have Pod structure in common and reuse it everywhere? Later we can think about extracting more data to meta object.

@bryk what do you think?

---

Comments from Reviewable

[bryk]

Review status: 0 of 17 files reviewed at latest revision, 13 unresolved discussions.

---

src/app/backend/handler/apihandler.go, line 191 [r7] (raw file):

Previously, floreks (Sebastian Florek) wrote…

@bryk should we also follow already new api concept? api/v1/deamonset/...?

We can do this later in a refectoring PR. Should be easy and we shouldn't be making this PR bigger.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 63 [r7] (raw file):

Previously, floreks (Sebastian Florek) wrote…

There is no deamon set specific data in this structure. Could we just have Pod structure in common and reuse it everywhere? Later we can think about extracting more data to meta object.

@bryk what do you think?

Yeah, that's what I'm doing. I hope I'll be able to merge my PR with common pod stuff asap and remove it from here. Sounds Okay?

---

Comments from Reviewable

[floreks]

Review status: 0 of 17 files reviewed at latest revision, 13 unresolved discussions.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 63 [r7] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Yeah, that's what I'm doing. I hope I'll be able to merge my PR with common pod stuff asap and remove it from here. Sounds Okay?

Perfect.

---

Comments from Reviewable

[Seb-Solon]

Review status: 0 of 15 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

src/app/backend/resource/common/podinfo.go, line 106 [r7] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Hmm... This needs a name and documentation that does not mention daemon sets.

I created this function because the type of Daemonset selector is *unversioned.LabelSelector (new , more generic selector AFAIK). Those Selectors have 2 fields : MatchExpressions and MatchLabels. As I did not know how to use MatchExpressions for now I've just copied the original behavior.

Do you want me to rename it to IsLabelSectorMatching and the other one IsSelectorMatching (because it actually uses Selector map[string]string) ?

---

src/app/backend/resource/common/podinfo.go, line 106 [r7] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Also, I think the argument semantics are flipped. I.e., "testedObjectLabels" is actually label selector and vice versa.

Nope, testedObject is always a daemonset.selector. I do a testedObjectLabels.MatchLabels in the function. Maybe you mean regarding the name of the variable? That make sense, I only stuck to the original function parameters

---

src/app/backend/resource/daemonset/daemonsetcommon.go, line 33 [r7] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Remove this struct and just use PodInfo from common/podinfo.go. It does the same.

I've added this because PodInfo have current and desired field that are are not used. But you are right I can use this without initializing those attributes

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 224 [r7] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Can you delete the internal/external endpoints code from this PR? We plan to have a common way of handling this across our codebase, so duplicating work is not necessary.

I am not sure of what you want. Do you want me to remove getServiceDetailforDS function and getExternalEndpointsforDS ? This will remove GetDaemonSetDetail also as it use the getServiceDetail

---

Comments from Reviewable

[Seb-Solon]

I'll squash my commit and rebase once we agree on the PR

[bryk]

A few more comments. I'm very happy where this PR goes. We should merge it soon.

My comments are mostly about using new stuff that was introduced in the backend code in the meantime.

Previously, Seb-Solon (Sébastien Coavoux) wrote…

I'll squash my commit and rebase once we agree on the PR

---

Review status: 0 of 15 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

src/app/backend/resource/common/podinfo.go, line 106 [r7] (raw file):

// Returns true when a Service with the given selector targets the same Pods (or subset) that
// a Daemon Set with the given selector.
func IsLabelSelectorMatchingforDS(labelSelector map[string]string,

I see.

Do you want me to rename it to IsLabelSectorMatching and the other one IsSelectorMatching (because it actually uses Selector map[string]string) ?

Yes, definitely :)

---

src/app/backend/resource/common/podinfo.go, line 106 [r7] (raw file):

Previously, Seb-Solon (Sébastien Coavoux) wrote…

Nope, testedObject is always a daemonset.selector. I do a testedObjectLabels.MatchLabels in the function. Maybe you mean regarding the name of the variable? That make sense, I only stuck to the original function parameters

Yeah, I meant name of the variable. Please rename and don't look at the old function (which may be wrong).

---

src/app/backend/resource/common/podinfo.go, line 109 [r7] (raw file):

  testedObjectLabels *unversioned.LabelSelector) bool {

  // If service has no selectors, then assume it targets different Pods.

s/service/something_else :)

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 224 [r7] (raw file):

Previously, Seb-Solon (Sébastien Coavoux) wrote…

I am not sure of what you want. Do you want me to remove getServiceDetailforDS function and getExternalEndpointsforDS ? This will remove GetDaemonSetDetail also as it use the getServiceDetail

So, the story is: Showing endpoints for replication controllers, replica sets, deamon sets is "controversial", since it has no formal definition in Kubernetes. We all know that this is endpoints for services with the same label selector, but this is not clearly communicated to the user.

But anyways, I think you can leave it here. We'll commonize it later.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 63 [r8] (raw file):

// Detailed information about a Pod that belongs to a Daemon Set
type DaemonSetPod struct {

Hey, @Seb-Solon I think the other PR with a common structure for pods has been merged. Can you rebase and use it here?

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 137 [r8] (raw file):

  }

  for _, pod := range pods.Items {

Now this code is commonized. Take a look at how RCs do this:

    replicationControllerDetail.Pods = pod.CreatePodList(pods.Items, heapsterClient)

Can you rebase and use new pattern?

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 241 [r8] (raw file):

  daemonSetPods := filterDaemonSetPods(daemonSet, pods)

  if service.Spec.Type == api.ServiceTypeNodePort {

Can you extract this to a common function with replicationcontrollerdetails.go? Looks like a copy&paste

---

src/app/backend/resource/daemonset/daemonsetlist.go, line 39 [r8] (raw file):

type DaemonSet struct {
  // Name of the Daemon Set
  Name string `json:"name"`

Can you use common.ObjectMeta and common.TypeMeta? Look how, e.g., deployments do this now.

I know this hasn't been there at the time you've been writing the code. We're coding a lot at currently in the project, so things are changing.

---

src/app/backend/resource/daemonset/daemonsetpodsmetrics.go, line 34 [r8] (raw file):

// Return Pods metrics for Daemon Set or error when occurred.
func getDaemonSetPodsMetrics(podList *api.PodList, heapsterClient client.HeapsterClient,

Afaik you can delete this file now and use common pods code. See how RC does this now.

---

Comments from Reviewable

[Seb-Solon]

I've rebased on master and used the pod resource available. I'll squash everything in 2/4 commits at the end (code + test or code + move + test *2) . Squashing will make tree more readable in my opinion, but maybe you don't mind having ~15 commits for that.

Previously, bryk (Piotr Bryk) wrote…

A few more comments. I'm very happy where this PR goes. We should merge it soon.

My comments are mostly about using new stuff that was introduced in the backend code in the meantime.

---

Review status: 0 of 13 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[Seb-Solon]

Review status: 0 of 13 files reviewed at latest revision, 7 unresolved discussions.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 241 [r8] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Can you extract this to a common function with replicationcontrollerdetails.go? Looks like a copy&paste

Well if I want to remove the code duplication here I would move the getExternalEndpoint into common package (types.go) next to the getInternalEnpoint one. In this case it would make sens to also move get*Enpoint from replicationcontroller package to common. Does this make sense in the daemonset pull request? Maybe another PR refactoring this is better. What do you think?

---

Comments from Reviewable

[floreks]

Review status: 0 of 13 files reviewed at latest revision, 7 unresolved discussions.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 160 [r8] (raw file):

// Deletes daemon set with given name in given namespace and related pods.
// Also deletes services related to daemon set if deleteServices is true.
func DeleteDaemonSet(client k8sClient.Interface, namespace, name string,

There is PR with generic delete functionality waiting for merge. Should be in soon so this can be refactored.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 241 [r8] (raw file):

Previously, Seb-Solon (Sébastien Coavoux) wrote…

Well if I want to remove the code duplication here I would move the getExternalEndpoint into common package (types.go) next to the getInternalEnpoint one. In this case it would make sens to also move get*Enpoint from replicationcontroller package to common. Does this make sense in the daemonset pull request? Maybe another PR refactoring this is better. What do you think?

I'll refactor replication controller code and make it more generic. You can then refactor your code and follow it. Possibly in next PR. @bryk what do you think?

---

Comments from Reviewable

[bryk]

Looks nice :) Please rebase, use all common functionality that @floreks mentioned and let's merge this and iterate on issues later. Please make next PRs smaller, so that we can have quicker turnaround :)

Previously, Seb-Solon (Sébastien Coavoux) wrote…

I've rebased on master and used the pod resource available. I'll squash everything in 2/4 commits at the end (code + test or code + move + test *2) . Squashing will make tree more readable in my opinion, but maybe you don't mind having ~15 commits for that.

---

Reviewed 3 of 11 files at r5, 3 of 12 files at r7, 3 of 15 files at r8, 2 of 8 files at r9, 8 of 12 files at r10.
Review status: 11 of 13 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

src/app/backend/resource/daemonset/daemonsetdetail.go, line 241 [r8] (raw file):

Previously, floreks (Sebastian Florek) wrote…

I'll refactor replication controller code and make it more generic. You can then refactor your code and follow it. Possibly in next PR. @bryk what do you think?

I agree. Makes sense. Just do anything to have this PR merged sooner :)

---

Comments from Reviewable

[Seb-Solon]

I am almost done but I still have a small issue.

The function GetExternalEndpoints here ( https://github.com/kubernetes/dashboard/blob/master/src/app/backend/resource/common/endpoint.go#L36) is not generic enough actually. DaemonSet selector are unversioned.LabelSelector with MatchExpressions and MatchLabels. If I want to use this function I must only use the MatchLabels attribute. I am not sure it's a good idea but we can fix it later. @bryk what do you think?

Previously, bryk (Piotr Bryk) wrote…

Looks nice :) Please rebase, use all common functionality that @floreks mentioned and let's merge this and iterate on issues later. Please make next PRs smaller, so that we can have quicker turnaround :)

---

Review status: 11 of 13 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[Seb-Solon]

I rebased and updated the code. I tried to used the generic function as much as I could except for the part I was referring above. This forced me to export more functions from common package to use them

Previously, Seb-Solon (Sébastien Coavoux) wrote…

I am almost done but I still have a small issue.

The function GetExternalEndpoints here ( https://github.com/kubernetes/dashboard/blob/master/src/app/backend/resource/common/endpoint.go#L36) is not generic enough actually. DaemonSet selector are unversioned.LabelSelector with MatchExpressions and MatchLabels. If I want to use this function I must only use the MatchLabels attribute. I am not sure it's a good idea but we can fix it later. @bryk what do you think?

---

Review status: 1 of 15 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[bryk]

The function GetExternalEndpoints here ( https://github.com/kubernetes/dashboard/blob/master/src/app/backend/resource/common/endpoint.go#L36) is not generic enough actually. DaemonSet selector are unversioned.LabelSelector with MatchExpressions and MatchLabels. If I want to use this function I must only use the MatchLabels attribute. I am not sure it's a good idea but we can fix it later. @bryk what do you think?

Create an issue on GitHub that the generic method is invalid and go ahead and use it. State in the bug how it should look like.

All else LGTM :)

---

Reviewed 3 of 11 files at r5, 2 of 12 files at r7, 1 of 15 files at r8, 12 of 17 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[Seb-Solon]

Done with #743. Commits squashed.

Previously, bryk (Piotr Bryk) wrote…

The function GetExternalEndpoints here ( https://github.com/kubernetes/dashboard/blob/master/src/app/backend/resource/common/endpoint.go#L36) is not generic enough actually. DaemonSet selector are unversioned.LabelSelector with MatchExpressions and MatchLabels. If I want to use this function I must only use the MatchLabels attribute. I am not sure it's a good idea but we can fix it later. @bryk what do you think?

Create an issue on GitHub that the generic method is invalid and go ahead and use it. State in the bug how it should look like.

All else LGTM :)

---

Review status: 9 of 12 files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[bryk]

Previously, Seb-Solon (Sébastien Coavoux) wrote…

Done with #743. Commits squashed.

---

Reviewed 2 of 11 files at r5, 1 of 12 files at r7, 3 of 6 files at r13.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bryk]

Merging :) Thanks for the work. Do you need any help now?

Previously, bryk (Piotr Bryk) wrote…

:lgtm:

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable