rebase 1.9.0 beta.1 #17576

deads2k · 2017-12-04T15:42:55Z

Green:

networking
verify
cross
integration
end-to-end with router removed
unit
test-cmd
extended builds
extended image ecosystem

Current counts:

GCE - 5 failures - 4 networking failures
extended conformance - 2 failures
end-to-end router tests - CI can't run e2e tests anymore because its trying to use golang 1.8.3. I removed them

e2e failures on gce:

[Feature:DeploymentConfig] deploymentconfigs when run iteratively [Conformance] should only deploy the last deployment [Suite:openshift/conformance/parallel] - flaking
[sig-network] Networking Granular Checks: Pods should function for intra-pod communication: http [Conformance] [Suite:openshift/conformance/parallel] [Suite:k8s] 5m27s
[sig-network] Networking Granular Checks: Pods should function for intra-pod communication: udp [Conformance] [Suite:openshift/conformance/parallel] [Suite:k8s] 5m43s
[sig-network] Networking Granular Checks: Pods should function for node-pod communication: http [Conformance] [Suite:openshift/conformance/parallel] [Suite:k8s] 5m50s
[sig-network] Networking Granular Checks: Pods should function for node-pod communication: udp [Conformance] [Suite:openshift/conformance/parallel] [Suite:k8s] 5m49s

e2e failures on install:

[Conformance][Area:Networking][Feature:Router] openshift router metrics The HAProxy router should expose prometheus metrics for a route [Suite:openshift/conformance/parallel] 21s @deads2k reproduced locally
[Conformance][Area:Networking][Feature:Router] openshift router metrics The HAProxy router should expose the profiling endpoints [Suite:openshift/conformance/parallel] 23s @deads2k reproduced locally

flaky failures

[Feature:Builds] Optimized image builds should succeed [Conformance] [Suite:openshift/conformance/parallel] 23s
[Feature:Builds][Conformance] build without output image building from templates should create an image from a docker template without an output image reference defined [Suite:openshift/conformance/parallel] 1m5s
[sig-storage] Downward API volume should update labels on modification [Conformance] [Suite:openshift/conformance/parallel] [Suite:k8s] 2m19s I think this flakes on master
[Feature:DeploymentConfig] deploymentconfigs when run iteratively [Conformance] should only deploy the last deployment [Suite:openshift/conformance/parallel] - flaking

Post-rebase tasks

deads2k · 2017-12-06T16:07:01Z

@sjenning there are significant changes to the node wiring. In particular kubernetes/kubernetes#53564, kubernetes/kubernetes#52287, and kubernetes/kubernetes#54405 . I've put your name in the commit where I deleted them in our config, but I'm not sure what will happen to us.

danwinship · 2017-12-07T16:18:34Z

pkg/cmd/server/kubernetes/network/options/options.go

@@ -64,6 +64,12 @@ func Build(options configapi.NodeConfig) (*kubeproxyconfig.KubeProxyConfiguratio
 	masqueradeBit := int32(0)
 	proxyconfig.IPTables.MasqueradeBit = &masqueradeBit

+	defaultedProxyConfig, err := proxyOptions.ApplyDefaults(proxyconfig)


This will end up calling SetDefaults_KubeProxyConfiguration() in pkg/proxy/apis/kubeproxyconfig/v1alpha1/defaults.go, which will force MetricsBindAddress and ResourceContainer to non-empty values, contradicting the values we set earlier in this function. Maybe you want to call this at the start rather than the end?

mfojtik · 2017-12-11T09:25:19Z

pkg/cmd/server/origin/controller/quota.go

 )

 func RunResourceQuotaManager(ctx ControllerContext) (bool, error) {
 	concurrentResourceQuotaSyncs := int(ctx.OpenshiftControllerOptions.ResourceQuotaOptions.ConcurrentSyncs)
 	resourceQuotaSyncPeriod := ctx.OpenshiftControllerOptions.ResourceQuotaOptions.SyncPeriod.Duration
 	replenishmentSyncPeriodFunc := calculateResyncPeriod(ctx.OpenshiftControllerOptions.ResourceQuotaOptions.MinResyncPeriod.Duration)
 	saName := "resourcequota-controller"
+	listerFuncForResource := generic.ListerFuncForResourceFunc(ctx.GenericInformerFunc)
+	quotaConfiguration := quotainstall.NewQuotaConfigurationForControllers(listerFuncForResource)


@derekwaynecarr guess this is enabling the generic quota count and fixes the bugzilla

soltysh · 2017-12-11T11:05:44Z

glide.lock

@@ -895,7 +918,7 @@ imports:
  subpackages:
  - imageprogress
 - name: github.com/openshift/source-to-image
-  version: e3e0eedc3f9dd55377ca2b8444769ec2ea54c895
+  version: 5bf2cb171c2cab1270fba80dc53ef4ff25b12c97


I wouldn't do that. This might cause some weird problems. If possible pin s2i to the current version we have. It can be easily bumped after rebase lands. Unless you strictly required something from newer version.

I wouldn't do that. This might cause some weird problems. If possible pin s2i to the current version we have. It can be easily bumped after rebase lands. Unless you strictly required something from newer version.

seems to be working.

soltysh · 2017-12-11T11:38:06Z

UPSTREAM: 00000: expose special storage locations
UPSTREAM: 00000: make the quota count reconcilation optional

Please open upstream PRs.

<drop>: skip scheduler configz error, drop once we run in a separate process
<drop>: skip controller metric error, drop once we run in a separate process

Is this something that will be handled in 3.9 timeframe? If yes add it to #17428.

Squash together:

pre-adjust moved packages 
boring: kapi moved 
boring: kapi moved    
boring: package move

soltysh · 2017-12-11T11:45:54Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/patch.go

+	"k8s.io/apimachinery/pkg/api/meta"
+)
+
+func PrintSuccess(mapper meta.RESTMapper, shortOutput bool, out io.Writer, resource, name string, dryRun bool, operation string) {


I see we rely on this function a lot in origin, but there's no reason to introduce a carry here. Why not copy it over to origin?

I see we rely on this function a lot in origin, but there's no reason to introduce a carry here. Why not copy it over to origin?

I'll let @juanvallejo sort it out when OC moves.

at least call it <drop> and add a comment about when we can drop it

soltysh · 2017-12-11T13:43:03Z

pkg/apps/registry/deployconfig/etcd/etcd.go

+}
+
+func (r *ScaleREST) GroupVersionKind(containingGV schema.GroupVersion) schema.GroupVersionKind {
+	switch containingGV {


@liggitt explicitly set us to use extensions/v1beta1.Scale only, not sure if this change is desired.

long term we want to move to autoscaling/Scale

but worth checking with @liggitt

@liggitt explicitly set us to use extensions/v1beta1.Scale only, not sure if this change is desired.

We need to make sure that any future version uses autoscaling. This code does that.

@soltysh this is not changing the default, if I do oc scale dc/foo I'm still send extensions/Scale and get extensions/Scale. I believe this is just so we have the right internal type.

Roger that.

soltysh · 2017-12-11T13:54:30Z

pkg/cmd/server/kubernetes/master/master_config.go

@@ -89,13 +89,15 @@ const originLongRunningEndpointsRE = "(/|^)(buildconfigs/.*/instantiatebinary|im

 var LegacyAPIGroupPrefixes = sets.NewString(apiserver.DefaultLegacyAPIPrefix, api.Prefix)

+// TODO I'm honestly not sure this is worth it. We're not likely to ever be able to launch from flags, so this just
+// adds a layer of complexity that is driving me crazy.


👍 with the increasing number of flags and the desire upstream to move to config file

sttts · 2017-12-13T16:11:21Z

hack/lib/util/golang.sh

@@ -11,7 +11,8 @@ function os::golang::verify_go_version() {
 	if [[ "${go_version[2]}" != go1.8* ]]; then
 		os::log::info "Detected go version: ${go_version[*]}."
 		if [[ -z "${PERMISSIVE_GO:-}" ]]; then
-			os::log::fatal "Please install Go version ${OS_REQUIRED_GO_VERSION} or use PERMISSIVE_GO=y to bypass this check."
+			os::log::warning "Please install Go version ${OS_REQUIRED_GO_VERSION} or use PERMISSIVE_GO=y to bypass this check."
+			return 0


not return 1?

Oh, this is intentional. Hmm. Ok.

not return 1?

No. The bash conditionals isn't right and something doesn't handle detection well. We hit this last time too.

liggitt · 2017-12-13T16:11:54Z

pkg/cmd/server/kubernetes/node/node.go

@@ -34,7 +34,7 @@ const minimumDockerAPIVersionWithPullByID = "1.22"
 // All errors here are fatal.
 func (c *NodeConfig) EnsureKubeletAccess() {
 	if _, err := os.Stat("/var/lib/docker"); os.IsPermission(err) {
-		c.HandleDockerError("Unable to view the /var/lib/docker directory - are you running as root?")


seems like we should be moving away from these checks as we lean more on cri, not making them unconditionally fatal

seems like we should be moving away from these checks as we lean more on cri, not making them unconditionally fatal

This is a pre-existing move. Not something net new.

or at least move this into EnsureDocker or protect with if c.KubeletServer.ContainerRuntime == "docker" { ... }

deads2k · 2017-12-13T16:13:04Z

/retest

sttts · 2017-12-13T16:14:47Z

pkg/image/registry/imagestreamtag/rest.go

 	} else {
 		if err := rest.BeforeUpdate(Strategy, ctx, obj, old); err != nil {
 			return nil, false, err
 		}
+		if err := updateValidation(obj.DeepCopyObject(), old.DeepCopyObject()); err != nil {


do we know old is non-nil? Aren't there ways to update without old object? Remembering a comment like that from @lavalamp in a different context. @liggitt ?

do we know old is nil? Aren't there ways to update without old object? Remembering a comment like that from @lavalamp in a different context. @liggitt ?

An update that allows create if it doesn't exist. imagestreamtags don't do that. Also, nil receivers are allowed and as I recall handled by the generated deep copy.

nil receivers for struct pointers. For interfaces they panic.

liggitt · 2017-12-13T16:14:54Z

pkg/cmd/server/kubernetes/node/node.go

-
-// HandleDockerError handles an an error from the docker daemon
-func (c *NodeConfig) HandleDockerError(message string) {
-	if !c.AllowDisabledDocker {


why remove this?

why remove this?

kubelet removed the ability to pass the client through, so the responsibility for wiring a fake falls to kubelet. We can't wire it through any more.

sttts · 2017-12-13T16:15:22Z

pkg/project/registry/project/proxy/proxy.go

@@ -167,6 +171,9 @@ func (s *REST) Update(ctx apirequest.Context, name string, objInfo rest.UpdatedO
 	if errs := s.updateStrategy.ValidateUpdate(ctx, obj, oldObj); len(errs) > 0 {
 		return nil, false, kerrors.NewInvalid(projectapi.Kind("Project"), project.Name, errs)
 	}
+	if err := updateValidation(obj.DeepCopyObject(), oldObj.DeepCopyObject()); err != nil {


sttts · 2017-12-13T16:16:50Z

pkg/authorization/registry/subjectaccessreview/registry.go

@@ -23,7 +24,7 @@ func NewRegistry(s Storage) Registry {
 }

 func (s *storage) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *api.SubjectAccessReview) (*api.SubjectAccessReviewResponse, error) {
-	obj, err := s.Create(ctx, subjectAccessReview, false)
+	obj, err := s.Create(ctx, subjectAccessReview, nil, false)


Didn't I add some DenyAlways funcs in upstream? Would make it more explicit.

deads2k · 2017-12-13T16:20:41Z

@ironcladlou @tnozicka hit the anti-flake on end-to-end and it passed

deads2k · 2017-12-13T16:21:15Z

Every GCE test has passed on this commit. Just not all at the same time.

liggitt · 2017-12-13T16:25:23Z

pkg/cmd/server/kubernetes/node/node_config_test.go

+			CloudProvider:           "", // now disabled
+			RootDirectory:           "/var/lib/kubelet",
+			CertDirectory:           "/var/lib/kubelet/pki",
+			RegisterNode:            true,                              // this looks suspicious


remove comment... this is ok

liggitt · 2017-12-13T16:30:38Z

pkg/cmd/server/kubernetes/node/node_config_test.go

+			RootDirectory:           "/var/lib/kubelet",
+			CertDirectory:           "/var/lib/kubelet/pki",
+			RegisterNode:            true,                              // this looks suspicious
+			RemoteRuntimeEndpoint:   "unix:///var/run/dockershim.sock", // overridden


add Containerized: false, // overridden based on OPENSHIFT_CONTAINERIZED

deads2k · 2017-12-13T16:33:15Z

/retest

deads2k · 2017-12-13T17:57:01Z

all but the router health error code have passed on conformance install.

liggitt · 2017-12-13T17:58:31Z

pkg/cmd/server/kubernetes/node/node_config_test.go

@@ -151,7 +147,7 @@ func TestKubeletDefaults(t *testing.T) {
 	}

 	if goruntime.GOOS == "darwin" {
-		expectedDefaults.KubeletConfiguration.RemoteRuntimeEndpoint = ""
+		//expectedDefaults.KubeletConfiguration.RemoteRuntimeEndpoint = ""


follow up to restore this, object.KubeletFlags.RemoteRuntimeEndpoint

liggitt · 2017-12-13T18:01:55Z

pkg/authorization/authorizer/authorizer.go

-	allowed, delegateReason, err := a.delegate.Authorize(attributes)
-	if allowed {
-		return true, reason(attributes), nil
+	authorized, delegateReason, err := a.delegate.Authorize(attributes)


this should return authorized, not convert it to Deny

this should return authorized, not convert it to Deny

done

openshift-ci-robot · 2017-12-13T18:03:24Z

@deads2k: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_gce	`b374cbd`	link	`/test extended_conformance_gce`
ci/openshift-jenkins/extended_conformance_install	`b374cbd`	link	`/test extended_conformance_install`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

liggitt · 2017-12-13T18:03:35Z

pkg/authorization/authorizer/authorizer.go

 	if attributes.GetUser() == nil {
-		return false, "", errors.New("no user available on context")
+		return authorizer.DecisionDeny, "", errors.New("no user available on context")


mechanical change to return NoOpinion, not Deny (applies everywhere)

mechanical change to return NoOpinion, not Deny (applies everywhere)

done

liggitt · 2017-12-13T18:07:03Z

pkg/authorization/registry/localsubjectaccessreview/rest_test.go

@@ -259,7 +259,7 @@ func (r *subjectAccessTest) runTest(t *testing.T) {

 	expectedResponse := &authorizationapi.SubjectAccessReviewResponse{
 		Namespace:       r.reviewRequest.Action.Namespace,
-		Allowed:         r.authorizer.allowed,
+		Allowed:         r.authorizer.allowed == kauthorizer.DecisionAllow,


Denied: r.authorizer.allowed == kauthorizer.DecisionDeny,

Denied: r.authorizer.allowed == kauthorizer.DecisionDeny,

Not for our object.

liggitt · 2017-12-13T18:27:26Z

pkg/oc/admin/migrate/storage/storage.go

@@ -161,6 +164,9 @@ func NewCmdMigrateAPIStorage(name, fullName string, f *clientcmd.Factory, in io.
 		Long:    internalMigrateStorageLong,
 		Example: fmt.Sprintf(internalMigrateStorageExample, fullName),
 		Run: func(cmd *cobra.Command, args []string) {
+			apiextensioninstall.Install(legacyscheme.GroupFactoryRegistry, legacyscheme.Registry, legacyscheme.Scheme)
+			apiregistrationinstall.Install(legacyscheme.GroupFactoryRegistry, legacyscheme.Registry, legacyscheme.Scheme)


need a follow up to figure out what we need to do to revert this... migrate shouldn't need registered types... it reads into unstructured and puts back

need a follow up to figure out what we need to do to revert this... migrate shouldn't need registered types... it reads into unstructured and puts back

done

liggitt · 2017-12-13T18:28:21Z

pkg/oc/admin/migrate/migrator.go

@@ -215,7 +215,8 @@ func (o *ResourceOptions) Complete(f *clientcmd.Factory, c *cobra.Command) error
 		break
 	}

-	o.Builder = f.NewBuilder(true).
+	o.Builder = f.NewBuilder().
+		Internal().


revert this... we have to deal with unstructured objects in migrate

or maybe make Unstructured()... not sure

revert this... we have to deal with unstructured objects in migrate

Not according to the old code. Looks like it was always going into internal types.

liggitt · 2017-12-13T18:53:27Z

/lgtm

can take comments as follow ups.

need immediate update to disable two router tests to unblock queue, then PR to re-enable and add debugging to troubleshoot the failures.

liggitt · 2017-12-13T18:54:19Z

all tests (except two router tests) passed at least once on this commit level. no conflicts with master. merging with immediate follow ups.

openshift-ci-robot · 2017-12-13T18:54:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, mfojtik

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~~ [deads2k,liggitt,mfojtik]

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

sttts · 2017-12-14T09:22:07Z

🎉 🚀 🎈

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 4, 2017

openshift-ci-robot requested review from smarterclayton and soltysh December 4, 2017 15:43

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 4, 2017

openshift-merge-robot added the needs-api-review label Dec 4, 2017

deads2k force-pushed the rebase-1.9.0-beta.1 branch from 1c592e9 to aaeb330 Compare December 4, 2017 15:49

soltysh mentioned this pull request Dec 5, 2017

Rename deploy.* imports to apps.* #16281

Merged

deads2k force-pushed the rebase-1.9.0-beta.1 branch from aaeb330 to 1557bb3 Compare December 5, 2017 21:11

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2017

soltysh mentioned this pull request Dec 6, 2017

Add --selector, --pod-selector flags oc adm drain #17616

Merged

deads2k force-pushed the rebase-1.9.0-beta.1 branch from 8b12ec9 to 254ba8a Compare December 6, 2017 15:50

deads2k force-pushed the rebase-1.9.0-beta.1 branch 2 times, most recently from af4137f to 6c1e6e1 Compare December 6, 2017 20:04

This was referenced Dec 6, 2017

UPSTREAM: 52917: add --selector support to drain,cordon #16445

Closed

UPSTREAM: 48763: Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set #15140

Closed

danwinship reviewed Dec 7, 2017

View reviewed changes

deads2k force-pushed the rebase-1.9.0-beta.1 branch 4 times, most recently from 6cc848c to 89dcb16 Compare December 8, 2017 14:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2017

deads2k force-pushed the rebase-1.9.0-beta.1 branch from 89dcb16 to c41b903 Compare December 8, 2017 16:00

mfojtik reviewed Dec 11, 2017

View reviewed changes

soltysh reviewed Dec 11, 2017

View reviewed changes

sttts reviewed Dec 13, 2017

View reviewed changes

liggitt reviewed Dec 13, 2017

View reviewed changes

sttts reviewed Dec 13, 2017

View reviewed changes

liggitt reviewed Dec 13, 2017

View reviewed changes

sttts reviewed Dec 13, 2017

View reviewed changes

liggitt reviewed Dec 13, 2017

View reviewed changes

openshift-ci-robot assigned liggitt Dec 13, 2017

liggitt merged commit a2ec324 into openshift:master Dec 13, 2017

dmage mentioned this pull request Dec 14, 2017

Image resource quota should deny a push of built image exceeding openshift.io/imagestreams quota #17786

Open

sjenning mentioned this pull request Jan 11, 2018

oc cluster up --service-catalog=true timing out on web-console #18046

Closed

deads2k deleted the rebase-1.9.0-beta.1 branch January 24, 2018 14:38

php-coder mentioned this pull request Mar 28, 2018

TestValidateSecurityContextConstraints: fix "index out of range" panic when there no errors encountered #19123

Merged

rebase 1.9.0 beta.1 #17576

rebase 1.9.0 beta.1 #17576

Conversation

deads2k commented Dec 4, 2017 • edited by mrogers950 Loading

deads2k commented Dec 6, 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

soltysh commented Dec 11, 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

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

soltysh Dec 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Dec 13, 2017

sttts Dec 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Dec 13, 2017

deads2k commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Dec 13, 2017

deads2k commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 13, 2017

liggitt Dec 13, 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

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

liggitt commented Dec 13, 2017

liggitt commented Dec 13, 2017

openshift-ci-robot commented Dec 13, 2017

sttts commented Dec 14, 2017

deads2k commented Dec 4, 2017 •

edited by mrogers950

Loading

deads2k commented Dec 6, 2017 •

edited

Loading

soltysh commented Dec 11, 2017 •

edited

Loading

soltysh Dec 11, 2017 •

edited

Loading

sttts Dec 13, 2017 •

edited

Loading

liggitt Dec 13, 2017 •

edited

Loading