Enhancement for enabling single-node-openshift with workers

[omertuc]

Reopening #996 since it got accidentally merged without sufficient
reviews

[omertuc]

/hold avoid accidental merge before we're satisfied with the reviews

[Miciah]

LGTM from the Network Edge side.

[tkashem]

LGTM from API extension side

[JoelSpeed]

I was asked to give this a thorough review from an API/UX perspective and as such you may notice a theme to a lot of my comments. The UX here does concern me, I'm not sure if we actually need this new field do we? It seems we should make users configure what they want rather than giving them a cryptic placement field (which I now understand only affects defaulting) which then affects the behaviour of another CR based on the combination of it and two other values - this seems very complex and I'm still not sure I understand it, I would wager my likelihood of understanding this is higher than that of the average user.

That said, I think there are ways we can improve the UX if we think carefully about how a user would interact with this feature.

I definitely think we should be adding an alternative (or maybe even main enhancement) that looks at just setting the defaulting based on existing fields rather than adding a new field, what are the pros/cons of that?

I also wanted to note that the content of this enhancement was not really what I was expecting from the PR description/title etc. There's very little about adding workers to SNO (in fact this seems to be a non-goal) so perhaps updating the naming of this might make it a bit easier to understand, this very much seems to be about "Allow configuration of placement of Ingress Pods" rather than anything to do with SNO workers, might be easier for other reviewers to understand if the title/description/etc were updated to reflect this

[omertuc]

I was asked to give this a thorough review from an API/UX perspective and as such you may notice a theme to a lot of my comments. The UX here does concern me, I'm not sure if we actually need this new field do we? It seems we should make users configure what they want rather than giving them a cryptic placement field (which I now understand only affects defaulting) which then affects the behaviour of another CR based on the combination of it and two other values - this seems very complex and I'm still not sure I understand it, I would wager my likelihood of understanding this is higher than that of the average user.

That's fair criticism. The motivation for this new field was to give the installer a lever it can pull to influence how the Ingress Operator initializes the default IngressController it creates, or how the Ingress Operator would initialize parameters missing from the installer-provided IngressController (the installer provides its own IngressController under some circumstances, but with some fields omitted, because it doesn't have an opinion on them)

An alternative would be to have the Installer create the IngressController in an opinionated way directly, rather than pulling this new lever, but that idea was dismissed because it requires the installer to have too much knowledge about IngressController CRs and how they work.

That said, I think there are ways we can improve the UX if we think carefully about how a user would interact with this feature.

I definitely think we should be adding an alternative (or maybe even main enhancement) that looks at just setting the defaulting based on existing fields rather than adding a new field, what are the pros/cons of that?

This was a path we explored - having the IngressController make decisions based on existing information, but it was shot down, for reasons I don't remember exactly. I'd have to dig through the comments of the previous PR to remember why. I'll reply to this comment once I find that.

I also wanted to note that the content of this enhancement was not really what I was expecting from the PR description/title etc. There's very little about adding workers to SNO (in fact this seems to be a non-goal) so perhaps updating the naming of this might make it a bit easier to understand, this very much seems to be about "Allow configuration of placement of Ingress Pods" rather than anything to do with SNO workers, might be easier for other reviewers to understand if the title/description/etc were updated to reflect this

The goal of this enhancement is very much to enable adding workers to single node - it just does so in a more "generic" way that isn't specific to single-node. A lot of that generality came after-the-fact, but the enhancement title stayed the same. The enhancement also slightly deals with the missing worker.ign in bip installations, which is a small topic but still very "sno+workers"-specific, so it made sense to keep the title, since it's solving both of those problems.

Also, the entire "alternatives" section deals with how we might have solved the SNO+Workers issue in a different ways that have nothing to do with this solution (e.g. having SNO support the baremetal platform), so it would be unfair to change the title

[JoelSpeed]

This was a path we explored - having the IngressController make decisions based on existing information, but it was shot down, for reasons I don't remember exactly. I'd have to dig through the comments of the previous PR to remember why. I'll reply to this comment once I find that.

If it's already been discussed great, I don't think I saw the last enhancement, or at least, for a long time. If you can find the conversation and distill the alternative into a paragraph in the alternatives section with the reasonings we don't think that should be done, that would be helpful

Also, the entire "alternatives" section deals with how we might have solved the SNO+Workers issue in a different ways that have nothing to do with this solution (e.g. having SNO support the baremetal platform), so it would be unfair to change the title

Fair comment, I guess my suggestion to make things easier would be to break this into two. The ingress part of this seems like a very well contained/scoped conversation. If that was one enhancement, and then the ignition parts and general information were another, depending on the ingress one, I think it might be easier to follow and have focused conversations

[cgwalters]

Please add @cgwalters for the MCO review as Sinny is away on extended leave

I only skimmed this so this is a relatively superficial thing but looks sane to me.

Unless I missed it, I didn't see much MCO implications?

[aravindhp]

Unless I missed it, I didn't see much MCO implications?

@cgwalters, Sinny was requested to review this so I roped you in.

[romfreiman]

@JoelSpeed @staebler @Miciah @cgwalters @aravindhp @dhellmann unless there are critical points, I expect this enhancement to be merged by tomorrow. Please start approving

[JoelSpeed]

From an API perspective, I'm happy to approve the PR. Will leave the labelling for others since there seem to be some outstanding reviews from other teams

@omertuc Please make sure the API PR is updated to reflect the changes outlined in the current version of the enhancement

[romfreiman]

@JoelSpeed approve label please?

[JoelSpeed]

@romfreiman I think the final approval should be left to @aravindhp once he is content that all the relevant teams have been appropriately consulted, there have been a number of changes since the installer and network edge teams left their LGTMs so I would expect at least @Miciah and @staebler to add a quick ack that they are still happy here

Aravindh, please consider the API approved for now, additional points may come up when it gets to the API PR but they can be amended retrospectively here

[staebler]

LGTM

[romfreiman]

@aravindhp as per my understanding, there are no open items. Can the enhancement be merged please?

[Miciah]

LGTM.

[aravindhp]

@aravindhp as per my understanding, there are no open items. Can the enhancement be merged please?

@romfreiman I was made aware about this enhancement only yesterday. I need some more time to go through it, so I will only get to it next week.

[dhellmann]

I've signed up as approver for this enhancement. (@omertuc that involved committing a change to the PR, so you'll want to fetch the new version before making updates)

We have a few unresolved threads, and it would be good to make sure the questions raised there have been answered. I commented separately on a few of those.

There are some TODO items left inline in the body. Those can either be removed or moved to the open questions list.

I had a couple of clarifying questions about points in the doc, but nothing that is likely to hold up progress.

[dhellmann]

/hold cancel
/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, dhellmann

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:

• OWNERS [dhellmann]

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

[openshift-ci]

@omertuc: all tests passed!

Full PR test history. Your PR dashboard.

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.