Manifest files #3229

gambol99 · 2017-08-17T13:16:17Z

The current implementation uses flags embedded into the command line rather than argument which is more consistent with other manifests and kubeadm. Logging currently pushes all the logs into the host, which isn't really ideal with an ephemeral host model, users should be pushed to implement a proper remote logging stack to retain logs not ssh to boxes.

changed the manifest to use arguments rather than option flags
changed the logging to use journal rather than relying on host logging files

core@ip-10-250-34-21 ~ $ cat /etc/kubernetes/manifests/kube-apiserver.manifest  | grep -A 6 args
  - args:
    - --address=127.0.0.1
    - --admission-control=NamespaceLifecycle,LimitRanger,ServiceAccount,PersistentVolumeLabel,DefaultStorageClass,DefaultTolerationSeconds,ResourceQuota,PodTolerationRestriction
    - --allow-privileged=true
    - --anonymous-auth=false
    - --apiserver-count=3
    - --authorization-mode=RBAC

k8s-ci-robot · 2017-08-17T13:16:25Z

Hi @gambol99. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

chrislovecnm · 2017-08-17T16:13:16Z

Thanks for the PR

This is a big change :) There has been an issue in for a long time around getting the logs accessible via kubectl logs. I am kinda on the fence about this, because this is going to change what admins are use to a bunch. No more log files ;)

I like using journal, but again big change in my mind. Let me ponder a bit...

justinsb · 2017-08-17T17:40:51Z

pkg/flagbuilder/build_flags.go

@@ -29,8 +29,18 @@ import (
 	"github.com/golang/glog"
 )

-// BuildFlags builds flag arguments based on "flag" tags on the structure
+// BuildFlags returns a space seperated list arguments


We should probably mark this deprecated (just with a comment saying to prefer calling BuildFlagsList)

justinsb · 2017-08-17T17:41:34Z

pkg/flagbuilder/build_flags.go

 		return utils.SkipReflection
 	}
 	err := utils.ReflectRecursive(reflect.ValueOf(options), walker)
 	if err != nil {
-		return "", err
+		return []string{}, err


FYI, typically nil is basically equivalent to []string{} for all practical use cases, so you can typically just return nil, err

justinsb · 2017-08-17T17:51:16Z

I agree with the change @gambol99 , except that (like @chrislovecnm) I do have concerns about backwards compatibility.

One option might be the same trick I suggested for the IAM changes #3186 (comment) ... one day we'll enable a logging solution "out of the box" just like we do networking. Enabling one of those logging solutions won't be the default when we first introduce it, but if the user enables it they get this more sensible logging. And until then we have a feature flag.

I think the options are:

We decide that it is OK to "break" logging for these components, which seems unlikely to me
We log to both the local file and stdout, like we do with kube-proxy now. No need for a flag.
We have selective behaviour where we send it to one or both destinations, driven by a feature flag or field.

I'm inclined to suggest that we do the second option, but see if we can transform this PR into that because then it will make the 3rd option easier.

What do you think @gambol99 ? And which logging solution are you using?

gambol99 · 2017-08-21T15:03:33Z

hi @justinsb / @chrislovecnm ...

One option might be the same trick I suggested for the IAM changes #3186 (comment) ... one day we'll enable a logging solution "out of the box" just like we do networking. Enabling one of those logging solutions won't be the default when we first introduce it, but if the user enables it they get this more sensible logging. And until then we have a feature flag.

Off topic to the PR but i'm inclined to forgo Kops managing any of those deployments; I'm not sure but I kinda feel it should only manage addon's which are functional to the cluster (a CNI or kube-dns etc) i.e. operations. Anything which is "feature based" should be out of scope and be part of an application-pipeline. It reduces the conditionals, api spec, templating and things Kops need to think about.

I'm inclined to suggest that we do the second option, but see if we can transform this PR into that because then it will make the 3rd option easier.

Agreed!, I've reverted the changes and it will now log to both stdout and original logging file

And which logging solution are you using?

At present a standard ELK stack, although it lives externally and isolated to clusters

chrislovecnm · 2017-08-21T17:14:49Z

. Anything which is "feature based" should out of scope and be part of an application-pipeline. It reduces the conditionals, API spec, templating and things Kops need to think about.

Yes, I agree. The question is if logging is a base component or not. Since there are like 40 different solutions I am thinking no. @justinsb has been threatening to build a deployment manager. Dashboard, logging, an other stuff could go there.

We already have some of the functionality needed in channels.

Admittedly kops deploying other manifests has been a big discussion item.

justinsb · 2017-08-22T07:03:32Z

Yes, up until now the line has been whether we need a component in order to bring up the system. Networking therefore is in. kube2iam or IAM lockdown is not in today, but I could imagine that we might have a really locked down configuration in future where e.g. kube-controller-manager has different IAM permissions from kube-apiserver, and then maybe kube2iam would be required. The idea of baking kube2iam in was convenient for figuring out how to incrementally lock down IAM permissions, but I think we can just make it a separate field.

Logging hasn't been one that I thought should be in, except that it would have been convenient here to incrementally adopt the change. (And maybe for logging/diagnostics during system bootstrap).

And yes, my hope is that we can get all the addons to be a simple manifest, in which case this might become largely academic anyway.

justinsb · 2017-08-22T07:15:16Z

nodeup/pkg/model/kube_proxy.go

+	flags = append(flags, []string{
+		"--conntrack-max-per-core=131072",
+		"--kubeconfig=/var/lib/kube-proxy/kubeconfig",
+		"--oom_score_adj=-998",


Upstream added the code which did echo -998 > /proc/$$$/oom_score_adj && ..., so I opened kubernetes/kubernetes#51083 to ask upstream / @vishh

justinsb · 2017-08-22T07:24:05Z

/lgtm

Thanks for changing the behaviour @gambol99

justinsb · 2017-08-22T07:47:05Z

/ok-to-test

justinsb · 2017-08-22T22:02:43Z

/retest

gambol99 · 2017-08-22T22:57:18Z

this one needs fixing @justinsb, no need to retest yet... With the revert of logging to both pipes it's causing issues which need fixing :-( ...

justinsb · 2017-08-22T23:15:27Z

Fair enough - there is also an upstream issue - kubernetes/kubernetes#51128, hopefully fixed by kubernetes/kubernetes#51144

k8s-github-robot · 2017-08-22T23:16:06Z

@gambol99 PR needs rebase

eparis · 2017-08-23T00:03:20Z

/retest
merged the referenced fix

joelsmith · 2017-08-23T01:00:04Z

/test pull-kops-e2e-kubernetes-aws

gambol99 · 2017-08-28T15:11:38Z

sorry for the lack of update on this one .. I did hit a stumbling block trying to get them both to work at the same time ... Given the arguments are now being passed to the entrypoint the use of /bin/sh -c "<command | tee -a " will no longer work ... I tried fiddling with (tee -a /var/log/kube-apiserver.log) 2>&1; /usr/local/bin/kube-apiserver or exec > >(tee -a /var/log/kube-apiserver.log) 2>&1; /usr/local/bin/kube-apiserver and a plethora of others!! .. The only way i got it to work was almost criminal, getting nodeup to write an entrypoint.sh, map into the container by a host volume and change the entrypoint to that; performing a simple;

#!/bin/sh
logfile=$1; shift 1;
$@ | tee -a ${logfile}

Or just having an extra option in the cluster spec that permits a selection ...

gambol99 · 2017-09-22T10:57:49Z

@KashifSaadat ... can i get a review please

KashifSaadat · 2017-09-22T11:08:55Z

nodeup/pkg/model/kube_apiserver.go

-		c.AddTask(t)
-	}
+	c.AddTask(&nodetasks.File{
+		Path:        "/var/log/kube-apiserver.log",


Could possibly make this path a constant as you're referring to it in a few places (same for the other models)

KashifSaadat · 2017-09-22T11:13:15Z

nodeup/pkg/model/kube_proxy.go

+		Image: image,
+		Command: []string{
+			"/bin/sh", "-c",
+			"/usr/local/bin/kube-proxy " + strings.Join(sortedStrings(flags), " ") + " 2>&1 | /usr/bin/tee -a /var/log/kube-proxy.log",


Similar logic could be abstracted out to a helper function

KashifSaadat · 2017-09-22T11:15:15Z

pkg/apis/kops/cluster.go

@@ -61,8 +61,7 @@ type ClusterSpec struct {
 	MasterPublicName string `json:"masterPublicName,omitempty"`
 	// MasterInternalName is the internal DNS name for the master nodes
 	MasterInternalName string `json:"masterInternalName,omitempty"`
-	// The CIDR used for the AWS VPC / GCE Network, or otherwise allocated to k8s
-	// This is a real CIDR, not the internal k8s network
+	// The CIDR used for the AWS VPC / GCE Network, or otherwise allocated to k8s. This is a real CIDR, not the internal k8s network


"NetworkCIDR is the CIDR used for.."

KashifSaadat · 2017-09-22T11:15:46Z

pkg/apis/kops/cluster.go

-	// Alternative locations for files and containers
-	// This API component is under construction, will remove this comment
-	// once this API is fully functional.
+	// Alternative locations for files and containers; the API under construction, will remove this comment once this API is fully functional.


Comment start with "Assets.."

KashifSaadat · 2017-09-22T11:17:58Z

pkg/flagbuilder/build_flags.go

 		return utils.SkipReflection
 	}
 	err := utils.ReflectRecursive(reflect.ValueOf(options), walker)
 	if err != nil {
-		return "", err
+		return nil, err


Could add some context to the errors being returned

KashifSaadat · 2017-09-22T11:19:03Z

Added a few comments, nothing big though.

Looks good to me! 👍

chrislovecnm · 2017-09-22T23:50:04Z

Arg ... needs a rebase again ... DOH

Or github is confused.

k8s-github-robot · 2017-09-22T23:51:37Z

@gambol99 PR needs rebase

gambol99 · 2017-09-27T14:48:29Z

is anything outstanding on this one? @justinsb @chrislovecnm

The current kube manifest redirect all the logs into host located log files, this PR uses the tee command to pipe into both local logs (retaining the current) and docker stdout (which will be picked up by the journald or which every logging your using. Note also permits as to now need the logs via the kubectl command. - renamed some of the files to make things cleaner - redirecting the logs from the kubernetes components into local file and stdout - cleaned up any vetting or linting error i came across

chrislovecnm · 2017-09-27T14:54:16Z

Yah just needed a rebase :)

/lgtm

k8s-github-robot · 2017-09-27T14:54:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, justinsb

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm,justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-27T16:14:19Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-27T16:55:48Z

Automatic merge from submit-queue.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2017

justinsb reviewed Aug 17, 2017

View reviewed changes

justinsb self-assigned this Aug 17, 2017

gambol99 force-pushed the manifest_files branch 6 times, most recently from cceb3a4 to abd01a6 Compare August 21, 2017 14:45

justinsb reviewed Aug 22, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 22, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2017

gambol99 force-pushed the manifest_files branch from 472892f to a307199 Compare September 22, 2017 08:21

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2017

gambol99 force-pushed the manifest_files branch from a307199 to 03415a4 Compare September 22, 2017 10:18

KashifSaadat reviewed Sep 22, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2017

justinsb added this to the 1.8.0 milestone Sep 25, 2017

gambol99 force-pushed the manifest_files branch from 03415a4 to 553130e Compare September 25, 2017 23:53

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2017

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 25, 2017

gambol99 force-pushed the manifest_files branch from 553130e to ab57579 Compare September 27, 2017 14:46

gambol99 force-pushed the manifest_files branch from ab57579 to a7abb07 Compare September 27, 2017 14:49

k8s-ci-robot assigned chrislovecnm Sep 27, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2017

k8s-github-robot merged commit 383a37a into kubernetes:master Sep 27, 2017

gambol99 deleted the manifest_files branch February 22, 2019 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manifest files #3229

Manifest files #3229

gambol99 commented Aug 17, 2017

k8s-ci-robot commented Aug 17, 2017

chrislovecnm commented Aug 17, 2017

justinsb Aug 17, 2017

gambol99 Aug 21, 2017

justinsb Aug 17, 2017

gambol99 Aug 21, 2017

justinsb commented Aug 17, 2017

gambol99 commented Aug 21, 2017 •

edited

Loading

chrislovecnm commented Aug 21, 2017

justinsb commented Aug 22, 2017

justinsb Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

gambol99 commented Aug 22, 2017 •

edited

Loading

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

eparis commented Aug 23, 2017

joelsmith commented Aug 23, 2017

gambol99 commented Aug 28, 2017 •

edited

Loading

gambol99 commented Sep 22, 2017

KashifSaadat Sep 22, 2017 •

edited

Loading

KashifSaadat Sep 22, 2017

KashifSaadat Sep 22, 2017

KashifSaadat Sep 22, 2017

KashifSaadat Sep 22, 2017

KashifSaadat commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

k8s-github-robot commented Sep 22, 2017

gambol99 commented Sep 27, 2017

chrislovecnm commented Sep 27, 2017

k8s-github-robot commented Sep 27, 2017

k8s-github-robot commented Sep 27, 2017

k8s-github-robot commented Sep 27, 2017

Manifest files #3229

Manifest files #3229

Conversation

gambol99 commented Aug 17, 2017

k8s-ci-robot commented Aug 17, 2017

chrislovecnm commented Aug 17, 2017

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

justinsb commented Aug 17, 2017

gambol99 commented Aug 21, 2017 • edited Loading

chrislovecnm commented Aug 21, 2017

justinsb commented Aug 22, 2017

Choose a reason for hiding this comment

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

gambol99 commented Aug 22, 2017 • edited Loading

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

eparis commented Aug 23, 2017

joelsmith commented Aug 23, 2017

gambol99 commented Aug 28, 2017 • edited Loading

gambol99 commented Sep 22, 2017

KashifSaadat Sep 22, 2017 • edited Loading

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

KashifSaadat commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

k8s-github-robot commented Sep 22, 2017

gambol99 commented Sep 27, 2017

chrislovecnm commented Sep 27, 2017

k8s-github-robot commented Sep 27, 2017

k8s-github-robot commented Sep 27, 2017

k8s-github-robot commented Sep 27, 2017

gambol99 commented Aug 21, 2017 •

edited

Loading

gambol99 commented Aug 22, 2017 •

edited

Loading

gambol99 commented Aug 28, 2017 •

edited

Loading

KashifSaadat Sep 22, 2017 •

edited

Loading