Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

openstack: Create machine resources programmatically #1107

Merged
merged 5 commits into from
Jan 29, 2019
Merged

openstack: Create machine resources programmatically #1107

merged 5 commits into from
Jan 29, 2019

Conversation

flaper87
Copy link
Contributor

Create the machine yaml files using go types rather than go template. This makes testing easier, it guarantees support for a specific openstack actuator version, and allows for more complex logic during the generation.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2019
@flaper87
vendor: Add cluster-api-provider-openstack
0bc9fc9
@flaper87
openstack: Always set opts.Cloud
0892d2f 
If OS_CLOUD is not set in the environment, this part of the code will
fail with a NPE because neither OS_CLOUD nor opts.Cloud are set. Set
opts.Cloud to the value in Platform.
@flaper87
openstack: Replace master machine templates with go types
5e5186f
@flaper87
Copy link
Contributor Author

/assign @tomassedovic

@flaper87
Copy link
Contributor Author

/test tf-fmt

@flaper87
Copy link
Contributor Author

/test e2e-aws

tomassedovic
tomassedovic suggested changes Jan 22, 2019
View reviewed changes
Copy link
Contributor

@tomassedovic tomassedovic left a comment

Choose a reason for hiding this comment

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

Thanks, this looks pretty good! I'm still unable to test this end to end due to #1092.

So what I did was run openshift-install create manifests and compared the output with my previous machine definitions I had lying around.

I've noticed a few differences (comments inline), but since I can't test this at the moment, I don't know whether those need to be addressed or not.

pkg/asset/machines/openstack/machines.go
},*/
Image: osImage,
CloudName: platform.Cloud,
CloudsSecret: &corev1.SecretReference{Name: cloudsSecret},
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to generate:

cloudsSecret:
    name: openstack-credentials

but the previous machine template had just: cloudsSecret: "openstack-credentials":

https://github.com/openshift/installer/pull/1042/files#diff-50d413300f38115e1c37fec85d8d7865R47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct. Format has changed upstream

pkg/asset/machines/openstack/machines.go
return &openstackprovider.OpenstackProviderSpec{
TypeMeta: metav1.TypeMeta{
APIVersion: "openstackproviderconfig.k8s.io/v1alpha1",
Kind: "OpenstackProviderSpec",
Copy link
Contributor

Choose a reason for hiding this comment

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

The Kind used to be OpenStackMachineProviderConfig, though I suspect OpenstackProviderSpec is the correct value now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

pkg/asset/machines/openstack/machines.go
},
AvailabilityZone: az,
SecurityGroups: []string{role},
// TODO(flaper87): Trunk support missing. Need to add it back
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to trunk support, the new templates don't seem to have the region placement set:

https://github.com/openshift/installer/pull/1042/files#diff-50d413300f38115e1c37fec85d8d7865R50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check and add it if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, placement is not a valid key in the actuator: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/pkg/apis/openstackproviderconfig/v1alpha1/types.go#L30

@wking wking mentioned this pull request Jan 28, 2019
Merged
@flaper87 flaper87 changed the title WIP: openstack: Create machine resources programmatically openstack: Create machine resources programmatically Jan 28, 2019
@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 Jan 28, 2019
@flaper87
Copy link
Contributor Author

/hold cancel

@tomassedovic
Copy link
Contributor

I'm seeing the following issue in the openshift log on the bootstrap node:

$ journalctl -f -u openshift
Jan 28 16:41:53 ostest-bootstrap openshift.sh[3347]: kubectl create --filename ./99_openshift-cluster-api_worker-machineset.yaml failed. Retrying in 5 seconds...
Jan 28 16:41:59 ostest-bootstrap openshift.sh[3347]: The MachineSet "ostest-worker-" is invalid: metadata.name: Invalid value: "ostest-worker-": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

The /etc/mcc/bootstrap/99_openshift-cluster-api_worker-machineset.yaml file contains this:

apiVersion: v1
items:
- apiVersion: cluster.k8s.io/v1alpha1
  kind: MachineSet
  metadata:
    creationTimestamp: null
    labels:
      sigs.k8s.io/cluster-api-cluster: ostest
      sigs.k8s.io/cluster-api-machine-role: worker
      sigs.k8s.io/cluster-api-machine-type: worker
    name: ostest-worker-
    namespace: openshift-cluster-api
  spec:
    replicas: 3
    selector:
      matchLabels:
        sigs.k8s.io/cluster-api-cluster: ostest
        sigs.k8s.io/cluster-api-machineset: ostest-worker-
    template:
      metadata:
        creationTimestamp: null
        labels:
          sigs.k8s.io/cluster-api-cluster: ostest
          sigs.k8s.io/cluster-api-machine-role: worker
          sigs.k8s.io/cluster-api-machine-type: worker
          sigs.k8s.io/cluster-api-machineset: ostest-worker-

I guess the worker names are missing the unique suffix?

@tomassedovic
Copy link
Contributor

openshift/cluster-kube-controller-manager-operator#135 (comment)

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flaper87, tomassedovic

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 [flaper87,tomassedovic]

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

@flaper87
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 9e263e0 into openshift:master Jan 29, 2019
@wking wking mentioned this pull request Feb 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomassedovic tomassedovic tomassedovic requested changes

@russellb russellb Awaiting requested review from russellb

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@tomassedovic tomassedovic

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flaper87 @tomassedovic @openshift-ci-robot @openshift-merge-robot