Currently ConfigFileAndDefaultsToInternalConfig and
FetchConfigFromFileOrCluster are used to default and load InitConfiguration
from file or cluster. These two APIs do a couple of completely separate things
depending on how they were invoked. In the case of
ConfigFileAndDefaultsToInternalConfig, an InitConfiguration could be either
defaulted with external override parameters, or loaded from file.
With FetchConfigFromFileOrCluster an InitConfiguration is either loaded from
file or from the config map in the cluster.
The two share both some functionality, but not enough code. They are also quite
difficult to use and sometimes even error prone.
To solve the issues, the following steps were taken:
- Introduce DefaultedInitConfiguration which returns defaulted version agnostic
InitConfiguration. The function takes InitConfiguration for overriding the
defaults.
- Introduce LoadInitConfigurationFromFile, which loads, converts, validates and
defaults an InitConfiguration from file.
- Introduce FetchInitConfigurationFromCluster that fetches InitConfiguration
from the config map.
- Reduce, when possible, the usage of ConfigFileAndDefaultsToInternalConfig by
replacing it with DefaultedInitConfiguration or LoadInitConfigurationFromFile
invocations.
- Replace all usages of FetchConfigFromFileOrCluster with calls to
LoadInitConfigurationFromFile or FetchInitConfigurationFromCluster.
- Delete FetchConfigFromFileOrCluster as it's no longer used.
- Rename ConfigFileAndDefaultsToInternalConfig to
LoadOrDefaultInitConfiguration in order to better describe what the function
is actually doing.
Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
The messages for container lifecycle events are subtly inconsistent
and should be unified.
First, the field format for containers is hard to parse for a human,
so include the container name directly in the message for create
and start, and for kill remove the container runtime prefix.
Second, the pulling image event has inconsistent capitalization, fix
that to be sentence without punctuation.
Third, the kill container event was unnecessarily wordy and inconsistent
with the create and start events. Make the following changes:
* Use 'Stopping' instead of 'Killing' since kill is usually reserved for
when we decide to hard stop a container
* Send the event before we dispatch the prestop hook, since this is an
"in-progress" style event vs a "already completed" type event
* Remove the 'cri-o://' / 'docker://' prefix by printing the container
name instead of id (we already do that replacement at the lower level
to prevent high cardinality events)
* Use 'message' instead of 'reason' as the argument name since this is a
string for humans field, not a string for machines field
* Remove the hash values on the container spec changed event because no
human will ever be able to do anything with the hash value
* Use 'Stopping container %s(, explanation)?' form without periods to
follow event conventions
The end result is a more pleasant message for humans:
```
35m Normal Created Pod Created container
35m Normal Started Pod Started container
10m Normal Killing Pod Killing container cri-o://installer:Need to kill Pod
10m Normal Pulling Pod pulling image "registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-10-172026@sha256:3da5303d4384d24691721c1cf2333584ba60e8f82c9e782f593623ce8f83ddc5"
```
becomes
```
35m Normal Created Pod Created container installer
35m Normal Started Pod Started container installer
10m Normal Killing Pod Stopping container installer
10m Normal Pulling Pod Pulling image "registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-10-172026@sha256:3da5303d4384d24691721c1cf2333584ba60e8f82c9e782f593623ce8f83ddc5"
```
The rules for address parsing are:
* Explicitly specified addresses must bind successfully
* `localhost` is pinned to `127.0.0.1` and `::1` and at least one of those must bind successfully
This change also makes output of the command consistent
between runs with the same arguments.
Previously the command was using the range via map of addresses
which sometimes was producing different output because the order
of values is not guaranteed in Go.
Move away from arrays to strings to fix several shellcheck-reported
issues. It isn't useful to expand the found files into arrays, because
only things that are checked are if the array is empty or the contents
of the first array item.
Fix also a shellcheck issue about using a literal string as regexp
match. It appears that the original reason for using a regexp was to
avoid specifying the directory in which the script is run. However, due
to the need of calling 'make generated_files', the directory is fixed
anyway, and the regexp can be left out.
Testing the change can be done with the following script which emulates
the different cases which the script can see. In the output the variable
'X' is the array and 'Z' is the string.
#!/bin/bash
set -o errexit
set -o nounset
set -o pipefail
function find_genfiles() {
find . \
\( \
-not \( \
\( \
-path ./_\* -o \
-path ./.\* \
\) -prune \
\) \
\) -name "$1"
}
# $1 = filename pattern as in "zz_generated.$1.go"
# $2 timestamp file
function newer() {
find_genfiles "$1" | while read -r F; do
if [[ "${F}" -nt "$2" ]]; then
echo "${F}"
fi
done
}
STAMP=stamp
mkdir -p xxx
touch xxx/foobar
touch "${STAMP}"
mkdir -p foo
touch foo/foobar
mkdir -p bar
touch bar/foobar
# two newer files
X=($(newer foobar "${STAMP}"))
if [[ "${#X[*]}" != 0 ]]; then
echo "X1:"
echo " ${X[*]:-(none)}"
fi
Z="$(newer foobar "${STAMP}")"
if [[ -n "$Z" ]]; then
echo "Z1:"
echo " ${Z}" | tr '\n' ' '
echo ""
fi
# no newer files
touch "${STAMP}"
X=($(newer foobar "${STAMP}"))
if [[ "${#X[*]}" != 0 ]]; then
echo "X2:"
echo " ${X[*]:-(none)}"
fi
Z="$(newer foobar "${STAMP}")"
if [[ -n "$Z" ]]; then
echo "Z2:"
echo " ${Z}" | tr '\n' ' '
echo ""
fi
# one newer file, name matches
touch "${STAMP}"
touch bar/foobar
X=($(newer foobar "${STAMP}"))
if [[ "${#X[@]}" != 1 || ! ( "${X[0]}" =~ "bar/foobar" ) ]]; then
echo "X3:"
echo " ${X[*]:-(none)}"
fi
Z="$(newer foobar "${STAMP}")"
if [[ -z "${Z}" || ${Z} != "./bar/foobar" ]]; then
echo "Z3:"
echo " ${Z:-(none)}" | tr '\n' ' '
echo ""
fi
# one newer file, name doesn't match
touch "${STAMP}"
touch foo/foobar
X=($(newer foobar "${STAMP}"))
if [[ "${#X[@]}" != 1 || ! ( "${X[0]}" =~ "bar/foobar" ) ]]; then
echo "X4:"
echo " ${X[*]:-(none)}"
fi
Z="$(newer foobar "${STAMP}")"
if [[ -z "${Z}" || ${Z} != "./bar/foobar" ]]; then
echo "Z4:"
echo " ${Z:-(none)}" | tr '\n' ' '
echo ""
fi
The expected output from running this script:
X1:
./bar/foobar ./foo/foobar
Z1:
./bar/foobar ./foo/foobar
X4:
./foo/foobar
Z4:
./foo/foobar
TestDynamicProvisioning had multiple ways of choosing additional
checks:
- the PvCheck callback
- the builtin write/read check controlled by a boolean
- the snapshot testing
Complicating matters further, that builtin write/read test had been
more customizable with new fields `NodeSelector` and
`ExpectUnschedulable` which were only set by one particular test (see
https://github.com/kubernetes/kubernetes/pull/70941).
That is confusing and will only get more confusing when adding more
checks in the future. Therefore the write/read check is now a separate
function that must be enabled explicitly by tests that want to run it.
The snapshot checking is also defined only for the snapshot test.
The test that expects unschedulable pods now also checks for that
particular situation itself. Instead of testing it with two pods (the
behavior from the write/read check) that both fail to start, only a
single unschedulable pod is created.
Because node name, node selector and the `ExpectUnschedulable` were
only used for checking, it is possible to simplify `StorageClassTest`
by removing all of these fields.
Expect(err).NotTo(HaveOccurred()) is an anti-pattern in Ginkgo testing
because a test failure doesn't explain what failed (see
https://github.com/kubernetes/kubernetes/issues/34059). We avoid it
now by making the check function itself responsible for checking
errors and including more information in those checks.
When the provisioning test gets stuck, the log fills up with messages
about waiting for a certain pod to run. Now the pod names are
pvc-[volume-tester|snapshot]-[writer|reader] plus the random
number appended by Kubernetes. This makes it easier to see where the
test is stuck.
There is no need to check for empty strings, we can also directly
initialize structs with the value. The end result is the same when the
value is empty (empty string in the struct).
This addresses the two remaining change requests from
https://github.com/kubernetes/kubernetes/pull/69036:
- replace "csi-hostpath-v0" name check with capability
check (cleaner that way)
- add feature tag to "should create snapshot with defaults" because
that is an alpha feature
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
Even if snapshots are supported by the driver interface, the driver or
suite might still want to skip a particular test, so those checks
still need to be executed.
Kubernetes Prow Robot