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.

Sign up for GitHub

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

Jump to bottom

remove disabledfeatures from master config #19070

Merged
merged 2 commits into from
Mar 26, 2018
Merged

remove disabledfeatures from master config #19070

merged 2 commits into from
Mar 26, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 22, 2018

  1. FeatureBuilder was agreed to remove by @smarterclayton and @eparis .
  2. FeatureS2I was not used
  3. FeatureWebConsole doesn't make sense the webconsole isn't include here. Just don't install it. Also, it didn't control the exposure of the console
  4. openshift.io/resource used by image builder. Don't know when this was added, but it never passed validation here: ValidateDisabledFeatures

@openshift/api-review
@openshift/sig-master
/assign @mfojtik
/assign @soltysh

@openshift-ci-robot openshift-ci-robot added sig/master approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 22, 2018
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 22, 2018
soltysh
soltysh reviewed Mar 23, 2018
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits and a question wrt to the image change trigger controller.

pkg/cmd/openshift-controller-manager/controller/image.go
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "apps", Resource: "statefulsets"},
Informer: ctx.ExternalKubeInformers.Apps().V1beta1().StatefulSets().Informer(),
Store: ctx.ExternalKubeInformers.Apps().V1beta1().StatefulSets().Informer().GetIndexer(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it can you update all these to use the Apps().V1() for all of the above 3 (deployments, daemonsets and statefulsets).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it can you update all these to use the Apps().V1() for all of the above 3 (deployments, daemonsets and statefulsets).

Doesn't that ripple through the types in the controller too? I'd have to update all the trigger funcs, rigth?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll create a followup.

pkg/cmd/openshift-controller-manager/controller/image.go
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "batch", Resource: "cronjobs"},
Informer: ctx.ExternalKubeInformers.Batch().V2alpha1().CronJobs().Informer(),
Store: ctx.ExternalKubeInformers.Batch().V2alpha1().CronJobs().Informer().GetIndexer(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, but Batch().V1beta1().

pkg/cmd/openshift-controller-manager/controller/image.go
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
}
sources = append(sources, imagetriggercontroller.TriggerSource{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly worried we're enabling this, iirc this was explicitly disabled due to some problems. @tnozicka or @mfojtik mind remember some details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly worried we're enabling this, iirc this was explicitly disabled due to some problems. @tnozicka or @mfojtik mind remember some details.

It would have failed validation here, right? https://github.com/openshift/origin/pull/19070/files#diff-6b6e279f3c86e5edb0688c1ca449db56L241

pkg/cmd/server/origin/openshift_apiserver.go
@@ -102,8 +102,6 @@ type OpenshiftAPIExtraConfig struct {
ProjectRequestTemplate string
ProjectRequestMessage string

EnableBuilds bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@deads2k
Copy link
Contributor Author

deads2k commented Mar 23, 2018

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Mar 25, 2018

/retest

mfojtik
mfojtik reviewed Mar 26, 2018
View reviewed changes
pkg/cmd/openshift-controller-manager/controller/image.go
@@ -63,51 +54,41 @@ func (c *ImageTriggerControllerConfig) RunController(ctx ControllerContext) (boo
Reactor: &triggerdeploymentconfigs.DeploymentConfigReactor{Client: appsClient.Apps()},
},
}
if !c.HasBuilderEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deads2k was this even logically correct before? if has not builder enabled, then add trigger for build configs sounds weird to me?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess not :-) and nobody noticed this since 3.6... shame on me and shame on @smarterclayton who manually merged it ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvmd. false alarm, this code is unfortunate but it works because we check if the feature is disabled when we initialize this struct (the negation should have been there to make this more clear...)

mfojtik
mfojtik reviewed Mar 26, 2018
View reviewed changes
pkg/cmd/server/apis/config/v1/types.go
@@ -228,9 +228,6 @@ type MasterConfig struct {
// ControllerConfig holds configuration values for controllers
ControllerConfig ControllerConfig `json:"controllerConfig"`

// DisabledFeatures is a list of features that should not be started.
DisabledFeatures FeatureList `json:"disabledFeatures"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we give users a deprecation warning here and ignore that option? Might help migration if somebody used this before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It never even worked, so I don't think it makes any sense. I wonder if we have any docs about it at all.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2018
soltysh
soltysh approved these changes Mar 26, 2018
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

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

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit e03c8a6 into openshift:master Mar 26, 2018
@enj enj mentioned this pull request Mar 26, 2018
Merged
@wozniakjan wozniakjan mentioned this pull request Jun 27, 2018
Merged
@deads2k deads2k deleted the server-08-features-on branch July 3, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

@soltysh soltysh soltysh approved these changes

@0xmichalis 0xmichalis Awaiting requested review from 0xmichalis

Assignees

@mfojtik mfojtik

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-api-review sig/master size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@deads2k @openshift-ci-robot @openshift-merge-robot @mfojtik @soltysh