Skip to content

Conversation

@adduarte
Copy link

@adduarte adduarte commented Oct 8, 2020

Adds mchineset controller.
The controller adds the diskKey, cpuKey, and memoryKey annotataions.
The annotations are required for autoscaling

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2020
@adduarte adduarte force-pushed the machineset_annotation_controller branch from 4845ecc to 59ff428 Compare October 14, 2020 05:52
@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/prow/gofmt 59ff428 link /test gofmt
ci/prow/unit 59ff428 link /test unit
ci/prow/images 59ff428 link /test images
ci/prow/e2e-openstack 59ff428 link /test e2e-openstack

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.

@adduarte adduarte self-assigned this Oct 15, 2020
@adduarte adduarte force-pushed the machineset_annotation_controller branch from 59ff428 to a4e8b04 Compare October 22, 2020 06:34
@adduarte adduarte force-pushed the machineset_annotation_controller branch 3 times, most recently from 7428861 to a7c831d Compare October 23, 2020 07:30
@adduarte adduarte changed the title [WIP] Adds scaling annotations to machineset Adds scaling annotations to machineset Oct 23, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2020
@adduarte
Copy link
Author

/approved

@adduarte adduarte force-pushed the machineset_annotation_controller branch 2 times, most recently from dbbb61f to 51a765d Compare October 23, 2020 14:43
@adduarte adduarte requested a review from iamemilio October 23, 2020 14:46
@adduarte
Copy link
Author

/approved

@openshift openshift deleted a comment from openshift-ci-robot Oct 26, 2020
@openshift openshift deleted a comment from openshift-ci-robot Oct 26, 2020
Copy link
Member

@EmilienM EmilienM left a comment

Choose a reason for hiding this comment

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

Some nits but otherwise it looks good to me; however I don't feel confident enough yet to approve it, there are a lot if internals that I need to learn before.

@iamemilio
Copy link

I think it would be very valuable to add a unit test for the cache.go library. Especially because it is async

The unit tests dont need to be too crazy, we just want to test:

  1. the staled and failure timeouts are respected
  2. getFlavorInfo() is thread safe
  3. Failing to find a flavor nil's the pointer in the cache
  4. our base case: returns flavor info when in cache or queries openstack

@adduarte adduarte force-pushed the machineset_annotation_controller branch from 51a765d to ba9570f Compare November 3, 2020 03:11
@adduarte adduarte requested review from Fedosin and iamemilio November 3, 2020 03:12
@adduarte adduarte force-pushed the machineset_annotation_controller branch from ba9570f to e53630e Compare November 4, 2020 08:14
@adduarte adduarte dismissed Fedosin’s stale review November 9, 2020 07:08

see inline comment.

@adduarte adduarte force-pushed the machineset_annotation_controller branch 2 times, most recently from 90d00a9 to 247de3b Compare November 11, 2020 09:54
Adds mchineset controller.
The controller adds the   cpuKey and memoryKey annotataions.
The annotations are required for autoscaling
@adduarte adduarte force-pushed the machineset_annotation_controller branch from 247de3b to eda781a Compare November 11, 2020 10:08
@adduarte
Copy link
Author

/test e2e-openstack

@iamemilio
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
@adduarte
Copy link
Author

/lgtm
/approve

(just making it explicit :) )

@openshift-ci-robot
Copy link

@adduarte: you cannot LGTM your own PR.

Details

In response to this:

/lgtm
/approve

(just making it explicit :) )

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
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adduarte

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2020
@adduarte
Copy link
Author

/test e2e-openstack

@openshift-merge-robot openshift-merge-robot merged commit 155384b into openshift:master Nov 16, 2020
@pierreprinetti pierreprinetti deleted the machineset_annotation_controller branch December 5, 2020 22:41
pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024
Previously I forgot to add the cloud.conf to nodes.
To rectify this mistake, I made the following changes:

  * Write the cloud.conf on the nodes.
  * Write a kubeadm join configuration.
  * Make kubeadm join using this configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants