USHIFT-936, USHIFT-942: reconcile the internal and external config data structures by dhellmann · Pull Request #1442 · openshift/microshift

dhellmann · 2023-03-03T23:40:27Z

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

/assign @pmtk
/assign @pacevedom
/assign @copejon

openshift-ci-robot · 2023-03-03T23:42:03Z

@dhellmann: This pull request references USHIFT-936 which is a valid jira issue.

Details

In response to this:

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

/assign @pmtk
/assign @pacevedom

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.

openshift-ci-robot · 2023-03-03T23:42:47Z

@dhellmann: This pull request references USHIFT-936 which is a valid jira issue.

Details

In response to this:

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

/assign @pmtk
/assign @pacevedom

need to ensure that we never look for element 0 of an empty list

the read function needs to initialize the data structure with the defaults before reading the file to ensure we always have valid defaults

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.

dhellmann · 2023-03-04T12:32:48Z

Tests failing with

Mar 04 00:23:38 release-ci-ci-op-swi8jhnb-5da1b microshift[212294]: ??? F0304 00:23:38.868198  212294 run.go:46] Error in reading or validating configuration: Failed to validate configuration file /etc/microshift/config.yaml: network.serviceNetwork not filled in
[96](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_microshift/1442/pull-ci-openshift-microshift-main-e2e-router-smoke-test/1631802154794618880#1:build-log.txt%3A96)

openshift-ci-robot · 2023-03-05T00:05:14Z

@dhellmann: This pull request references USHIFT-936 which is a valid jira issue.

Details

In response to this:

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

/assign @pmtk
/assign @pacevedom

need to ensure that we never look for element 0 of an empty list

the read function needs to initialize the data structure with the defaults before reading the file to ensure we always have valid defaults

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.

openshift-ci-robot · 2023-03-05T00:05:20Z

@dhellmann: This pull request references USHIFT-936 which is a valid jira issue.

Details

In response to this:

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

/assign @pmtk
/assign @pacevedom

need to ensure that we never look for element 0 of an empty list

the read function needs to initialize the data structure with the defaults before reading the file to ensure we always have valid defaults

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.

dhellmann · 2023-03-06T15:56:33Z

/retest

pkg/config/files.go

pkg/config/config.go

openshift-ci-robot · 2023-03-10T21:54:02Z

@dhellmann: This pull request references USHIFT-936 which is a valid jira issue.

Details

In response to this:

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

As part of the reconciliation it was necessary to change the etcd.doStartupDefrag
field from a bool to a string so we can detect whether the user provides a value
and so we can set the default to a positive value appropriately.

/assign @pmtk
/assign @pacevedom

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.

Move data structures and related functions into their own files.

This change streamlines the reading and validation functions so that it is easier to get a Config instance with the defaults or with valid values read from a file. A new convenience function for loading the active configuration, ignoring a missing configuration file, is added. The k8s YAML parser is used to read the config file, instead of the default parser, to ensure support for `json:"-"` directives to ignore fields in the input. The unit tests are updated to parse input config instead of using a data structure. This more accurately simulates what happens when the real program reads the config if only part of the file is populated, and clarifies which part of the config each test scenario is checking. It also allows for more, smaller, scenarios to isolate configuration validation and defaulting failures. The validation for node name changing is only relevant when starting the server, so that is moved to the run command implementation to make the unit tests for config easier to understand. The show-config command implementation no longer needs to map between multiple configuration data structures, so streamline that code.

Track the values we read from the user-provided configuration file separately from those that are defaulted or computed from other settings. This allows us to apply rules to computed values, like ApiServer.SkipAddress, based on whether or not the user has overridden the default.

Use clearer names based on what they return.

We need to convert the hostname to a value suitable for use as a node name. Instead of doing that in many places, provide a method to return the modified value.

We always want to defragment on startup, so we do not need a flag.

The URL field is not user-configurable, so we don't need a function to parse it.

dhellmann · 2023-03-31T22:26:31Z

/hold cancel

I believe this is ready for review.

dhellmann · 2023-04-01T17:30:40Z

/test e2e-rpm-install

openshift-ci · 2023-04-01T17:30:43Z

@dhellmann: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test e2e-openshift-conformance-reduced
/test e2e-openshift-conformance-reduced-arm
/test e2e-reboot
/test e2e-reboot-arm
/test images
/test test-srpm
/test test-unit
/test verify

The following commands are available to trigger optional jobs:

/test e2e-greenboot
/test e2e-greenboot-arm
/test e2e-loadbalancer-smoke-test
/test e2e-loadbalancer-smoke-test-arm
/test e2e-router-smoke-test
/test e2e-router-smoke-test-arm

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-microshift-main-e2e-greenboot
pull-ci-openshift-microshift-main-e2e-greenboot-arm
pull-ci-openshift-microshift-main-e2e-loadbalancer-smoke-test
pull-ci-openshift-microshift-main-e2e-loadbalancer-smoke-test-arm
pull-ci-openshift-microshift-main-e2e-openshift-conformance-reduced
pull-ci-openshift-microshift-main-e2e-openshift-conformance-reduced-arm
pull-ci-openshift-microshift-main-e2e-reboot
pull-ci-openshift-microshift-main-e2e-reboot-arm
pull-ci-openshift-microshift-main-e2e-router-smoke-test
pull-ci-openshift-microshift-main-e2e-router-smoke-test-arm
pull-ci-openshift-microshift-main-images
pull-ci-openshift-microshift-main-test-unit
pull-ci-openshift-microshift-main-verify

Details

In response to this:

/test e2e-rpm-install

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.

eggfoobar · 2023-04-04T05:30:46Z

LGTM, I was able to pull this and run it with out any issues.

openshift-ci-robot · 2023-04-04T21:24:11Z

@dhellmann: This pull request references USHIFT-942 which is a valid jira issue.

Details

In response to this:

This PR eliminates some technical debt by merging the internal and
external representation of our configuration. The PR is structured with
several commits that should each work independently to make reviewing
the large number of changes simpler.

/assign @pmtk
/assign @pacevedom
/assign @copejon

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.

pmtk · 2023-04-05T08:26:27Z

pkg/config/files.go

+// Returns the default user config file if that exists, else the default global
+// config file, else the empty string.
+func findConfigFile() string {
+	userConfigFile, _ := homedir.Expand(DefaultUserConfigFile)


It just occurred to me that we wanted to remove user dir (microshift needs to be run as root anyway), but given the size of the PR and effort spent on reviewing it we might do it in another PR :)
Especially what you're doing here is just moving what already existed

pmtk · 2023-04-05T08:30:39Z

@eggfoobar gave +1. Also saw @copejon +1 on slack
/lgtm

openshift-ci · 2023-04-05T08:31:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, pacevedom, pmtk

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dhellmann,pacevedom,pmtk]

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

openshift-ci · 2023-04-05T09:39:31Z

@dhellmann: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-rpm-install	dbc63b48299f48eeb79f5411ca6c0957af9ad970	link	true	`/test e2e-rpm-install`

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci-robot · 2023-04-05T09:55:49Z

/retest-required

Remaining retests: 0 against base HEAD b5a4e1e and 2 for PR HEAD b0d3371 in total

openshift-ci bot assigned pacevedom and pmtk Mar 3, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2023

openshift-ci bot requested review from pmtk and stlaz March 3, 2023 23:43

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2023

dhellmann added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Mar 3, 2023

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch 2 times, most recently from ffbe3d5 to dbc63b4 Compare March 4, 2023 18:04

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 5, 2023

dhellmann mentioned this pull request Mar 6, 2023

DO NOT MERGE: bisecting ci failures #1448

Closed

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch 2 times, most recently from af6f0d1 to b6e7c83 Compare March 7, 2023 00:50

pliurh reviewed Mar 8, 2023

View reviewed changes

pkg/config/files.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch from b995020 to d82f739 Compare March 8, 2023 22:17

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

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch from d82f739 to bed0b88 Compare March 8, 2023 22:45

pliurh reviewed Mar 9, 2023

View reviewed changes

pkg/config/files.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch from bed0b88 to b9d1ecf Compare March 10, 2023 21:36

dhellmann changed the title ~~WIP: USHIFT-936: merge the internal and external config data structures~~ WIP: USHIFT-936: reconcile the internal and external config data structures Mar 10, 2023

dhellmann changed the title ~~WIP: USHIFT-936: reconcile the internal and external config data structures~~ USHIFT-936: reconcile the internal and external config data structures Mar 10, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2023

dhellmann added 4 commits March 31, 2023 17:51

USHIFT-936: merge MicroshiftConfig into Config

43510fa

USHIFT-936: split up large config.go

cffbb13

Move data structures and related functions into their own files.

USHIFT-936: show configuration on startup

5c14fcb

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch from 7b0f04e to edf801f Compare March 31, 2023 22:00

dhellmann added 7 commits March 31, 2023 18:07

USHIFT-936: rename config entry point functions

b022ede

Use clearer names based on what they return.

USHIFT-936: introduce CanonicalNodeName

cdc051d

We need to convert the hostname to a value suitable for use as a node name. Instead of doing that in many places, provide a method to return the modified value.

USHIFT-936: remove Etcd.DoStartupDefrag boolean

23272d2

We always want to defragment on startup, so we do not need a flag.

fix formatting of comments

af48087

USHIFT-936: simplify API server port handling

29dc798

The URL field is not user-configurable, so we don't need a function to parse it.

ETCD-403: log command line for starting etcd

b0d3371

dhellmann force-pushed the USHIFT-936-reconcile-config-data-structures branch from edf801f to b0d3371 Compare March 31, 2023 22:26

dhellmann changed the title ~~WIP: USHIFT-936, USHIFT-942: reconcile the internal and external config data structures~~ USHIFT-936, USHIFT-942: reconcile the internal and external config data structures Mar 31, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2023

copejon mentioned this pull request Apr 3, 2023

USHIFT-1031: fix golangci-lint reported issues #1605

Merged

pmtk reviewed Apr 5, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2023

openshift-merge-robot merged commit 989ecf3 into openshift:main Apr 5, 2023

microshift-rebase-script bot mentioned this pull request Sep 30, 2024

NO-ISSUE: rebase-main-4.18.0-0.nightly-2024-09-29-150556_amd64-2024-09-29_arm64-2024-09-29 #3996

Closed

Conversation

dhellmann commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 3, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 3, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Mar 4, 2023

Uh oh!

openshift-ci-robot commented Mar 5, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 5, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Mar 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 10, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Mar 31, 2023

Uh oh!

dhellmann commented Apr 1, 2023

Uh oh!

openshift-ci bot commented Apr 1, 2023

Uh oh!

eggfoobar commented Apr 4, 2023

Uh oh!

openshift-ci-robot commented Apr 4, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmtk Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmtk commented Apr 5, 2023

Uh oh!

openshift-ci bot commented Apr 5, 2023

Uh oh!

openshift-ci bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dhellmann commented Mar 3, 2023 •

edited

Loading

openshift-ci-robot commented Mar 3, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 3, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 5, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 5, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 10, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Apr 4, 2023 •

edited by openshift-ci bot

Loading

pmtk Apr 5, 2023 •

edited

Loading

openshift-ci bot commented Apr 5, 2023 •

edited

Loading