Skip to content

Conversation

smarterclayton
Copy link

@smarterclayton smarterclayton commented Mar 27, 2019

Example of how you'd merge the latest upstream into our master branch as a PR.

Requirements:

  1. Rebases require PRs, no exceptions
  2. Keep upstream branches pristine
  3. Allow carry patches.

The flow described below preserves the three above and adds the following guarantee:

  1. All of our upstream commits after a rebase sit on top of a merge patch that defines the "floor" and thus makes diffs easy of what our carries are.

This flow is suitable for automation and should be (we want specific commit messages, for example) like Rebase: Choosing <GIT DESCRIBE> <COMMIT_ID>

The rough flow is:

  1. identify the new upstream
  2. tell git that the new upstream supersedes our current state (creating a merge commit with -s ours)
  3. Add cherry-picks
  4. push to pull request
git checkout -b new_rebase openshift/master
git checkout -b tmp origin/master
git merge -m "Rebase to upstream $( git rev-parse --short=10 HEAD )" -s ours new_rebase
git checkout new_rebase
git merge tmp
# at this point new_rebase is the same as upstream, but knows that openshift/master is done
git diff --exit-code || echo "NOT EQUAL TO UPSTREAM"
git branch -D tmp
...
# add cherry-picks

git push YOUR_FORK new_rebase
git pull-request -b openshift:master

your pull request at this point is "everything new" and you get PR tests and the history is preserved

git diff origin/master openshift/master
# shows every upstream commit

gjtempleton and others added 30 commits October 24, 2018 20:50
…anup

Add 2 missing CA flags to param docs
Refactor - add factories for Recommender & ClusterFeedProvider
The goal is to allow customization of this logic
for different use-case and cloudproviders.
Add informational UncappedTarged field to VPA api.
The error argument was omitted.
Also refactor Balancing processor a bit to make it easily extensible.
In k8s 1.11, pod priority, and preemption is enabled by default.
The API is under `v1beta`, and they do not need to be enabled.
…-in-1.11

Update FAQ on overprovisioning to account for k8s 1.11
This is preparatory work for handling resource related
(stockout/quota-exceeded) error conditions in CA.
…odes-return-instance-struct-instead-instance-name-2063d

NodeGroup.Nodes() return Instance struct instead instance name
On local hardware I have not seen this test fail using the current
50ms timeout. On AWS/CI I see this fail occasionally; I built and
copied this test to an AWS node and run it repeatedly for ~1 hour. The
min was 5ms and the max was 268ms, so bumping the timeout to 500ms.

Signed-off-by: Andrew McDermott <[email protected]>
…ancing

Move nodegroup balancing to processor, add GKE-specific implementation
Recommender capps recommendation according to policy.
…stWaitForOp

gce: increase test timeout in TestWaitForOp
Modify execution_latency_seconds buckets
Fix broken link to VPA Admission Webhook readme
Pass on-event oomInfo without creating a new goroutine.
frobware and others added 24 commits March 29, 2019 16:22
…mp deps

Add a makefile target `revendor-test-e2e` that captures the runes to
bump the e2e test dependencies.
This is no longer applicable.
This reworks `TestNodeGroup{Increase,Decrease}Size()` to use subtests.
The change is to ensure we are testing both MachineSets and
MachineDeployments in all cases we construct a nodegroup.

By switching to subtests it is easier to see those places where
machine deployments are not being tested as the the output for each
test function should show that MachineDeployment and MachineSet is
listed in the test name output.

```console
--- PASS: TestNodeGroupIncreaseSize (0.61s)
    --- PASS: TestNodeGroupIncreaseSize/MachineSet (0.30s)
        --- PASS: TestNodeGroupIncreaseSize/MachineSet/errors_because_delta_is_negative (0.10s)
        --- PASS: TestNodeGroupIncreaseSize/MachineSet/errors_because_initial+delta_>_maxSize (0.10s)
        --- PASS: TestNodeGroupIncreaseSize/MachineSet/no_error_as_within_bounds (0.10s)
    --- PASS: TestNodeGroupIncreaseSize/MachineDeployment (0.30s)
        --- PASS: TestNodeGroupIncreaseSize/MachineDeployment/errors_because_delta_is_negative (0.10s)
        --- PASS: TestNodeGroupIncreaseSize/MachineDeployment/errors_because_initial+delta_>_maxSize (0.10s)
        --- PASS: TestNodeGroupIncreaseSize/MachineDeployment/no_error_as_within_bounds (0.10s)

--- PASS: TestNodeGroupDecreaseSize (0.61s)
    --- PASS: TestNodeGroupDecreaseSize/MachineSet (0.30s)
        --- PASS: TestNodeGroupDecreaseSize/MachineSet/errors_because_delta_is_positive (0.10s)
        --- PASS: TestNodeGroupDecreaseSize/MachineSet/errors_because_delta_exceeds_node_count (0.10s)
        --- PASS: TestNodeGroupDecreaseSize/MachineSet/errors_because_size+delta_>=_len(nodes) (0.10s)
    --- PASS: TestNodeGroupDecreaseSize/MachineDeployment (0.30s)
        --- PASS: TestNodeGroupDecreaseSize/MachineDeployment/errors_because_delta_is_positive (0.10s)
        --- PASS: TestNodeGroupDecreaseSize/MachineDeployment/errors_because_delta_exceeds_node_count (0.10s)
        --- PASS: TestNodeGroupDecreaseSize/MachineDeployment/errors_because_size+delta_>=_len(nodes) (0.10s)
```

No code outside of the unit tests has been changed.
This reworks `TestNodeGroupMachineSetDeleteNodes` to use subtests. The
change is to ensure we are testing both MachineSets and
MachineDeployments in all cases we construct a nodegroup.

By switching to subtests it is easier to see those places where
machine deployments are not being tested as the the output for each
test function should show that MachineDeployment and MachineSet is
listed in the test name output.

```console
--- PASS: TestNodeGroupDeleteNodes (0.20s)
    --- PASS: TestNodeGroupDeleteNodes/MachineSet (0.10s)
    --- PASS: TestNodeGroupDeleteNodes/MachineDeployment (0.10s)
```
This reworks TestControllerNodeGroups to use subtests. The change is
to ensure we are testing both MachineSets and MachineDeployments in
all cases we construct a nodegroup.

By switching to subtests it is easier to see those places where
machine deployments are not being tested as the the output for each
test function should show that MachineDeployment and MachineSet is
listed in the test name output.

```console
--- PASS: TestControllerNodeGroups (0.61s)
    --- PASS: TestControllerNodeGroups/MachineSet (0.30s)
        --- PASS: TestControllerNodeGroups/MachineSet/errors_with_bad_annotations (0.10s)
        --- PASS: TestControllerNodeGroups/MachineSet/success_with_zero_bounds (0.10s)
        --- PASS: TestControllerNodeGroups/MachineSet/success_with_positive_bounds (0.10s)
    --- PASS: TestControllerNodeGroups/MachineDeployment (0.30s)
        --- PASS: TestControllerNodeGroups/MachineDeployment/errors_with_bad_annotations (0.10s)
        --- PASS: TestControllerNodeGroups/MachineDeployment/success_with_zero_bounds (0.10s)
        --- PASS: TestControllerNodeGroups/MachineDeployment/success_with_positive_bounds (0.10s)
```
Simplify test setup using `newMachineController()`.
Switch to using subtests vis-a-vis using t.Logf() to identify the test
being run.
…okup

This reworks TestControllerNodeGroupForNodeLookup to use subtests. The
change is to ensure we are testing both MachineSets and
MachineDeployments in all cases we construct a nodegroup.

By switching to subtests it is easier to see those places where
machine deployments are not being tested as the the output for each
test function should show that MachineDeployment and MachineSet is
listed in the test name output.

```console
--- PASS: TestControllerNodeGroupForNodeLookup (0.61s)
    --- PASS: TestControllerNodeGroupForNodeLookup/MachineSet (0.30s)
        --- PASS: TestControllerNodeGroupForNodeLookup/MachineSet/lookup_is_nil_because_no_annotations (0.10s)
        --- PASS: TestControllerNodeGroupForNodeLookup/MachineSet/lookup_is_nil_because_scaling_range_==_0 (0.10s)
        --- PASS: TestControllerNodeGroupForNodeLookup/MachineSet/lookup_is_successful_because_scaling_range_>=_1 (0.10s)
    --- PASS: TestControllerNodeGroupForNodeLookup/MachineDeployment (0.30s)
        --- PASS: TestControllerNodeGroupForNodeLookup/MachineDeployment/lookup_is_nil_because_no_annotations (0.10s)
        --- PASS: TestControllerNodeGroupForNodeLookup/MachineDeployment/lookup_is_nil_because_scaling_range_==_0 (0.10s)
        --- PASS: TestControllerNodeGroupForNodeLookup/MachineDeployment/lookup_is_successful_because_scaling_range_>=_1 (0.10s)
```
All the others tests are not run in parallel and running this small
subset with t.Parallel() buys us nothing.
Moved all test utility & setup functions into
machineapi_controller_test.go.

In the vast majority of the unit tests you need a controller so it
makes sense to consolidate all the utilities in that file.
Check for explicit error mesages in TestNodeGroupIncreaseSize() and
TestNodeGroupDecreaseSize(). These functions now only test the
expected error cases on bad inputs. A follow-up PR will test for the
success case which needs additional test setup code.
This adds the --kubeconfig path to the autoscaler options.
@openshift-ci-robot
Copy link

@smarterclayton: PR needs rebase.

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 openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2019
@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/prow/gofmt 3ffad39 link /test gofmt
ci/prow/golint 3ffad39 link /test golint
ci/prow/govet 3ffad39 link /test govet
ci/prow/unit 3ffad39 link /test unit
ci/prow/images 3ffad39 link /test images
ci/prow/e2e-aws-operator 3ffad39 link /test e2e-aws-operator
ci/jenkins/e2e 3ffad39 link /test e2e
ci/prow/goimports 3ffad39 link /test goimports

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.