Replace references to tectonic with openshift #644

staebler · 2018-11-09T04:40:39Z

Changes for https://jira.coreos.com/browse/CORS-878.

There are still two types of references to tectonic.

Resources are still tagged with tectonicClusterID. I will change this to openshiftClusterID in a subsequent PR. The change requires corresponding changes in other repos. I wanted to isolate the change so that the changes in this PR are not held up.
The smoke tests still have references to tectonic because they appear to still use the old tectonic installer. I wasn't sure if there was ongoing work to update these smoke tests or whether we should delete the entire directory. If it is the former, then most if not all of the tectonic references should be removed as a side effect of that work. If it is the latter, then we can remove the smoke tests in a subsequent PR.

abhinavdahiya · 2018-11-09T20:31:57Z

@wking had a previous (can't remember where) suggestion to keep stuff void of both tectonic / openshift esp in tf templates...

wking · 2018-11-09T20:45:30Z

@wking had a previous (can't remember where) suggestion to keep stuff void of both tectonic / openshift esp in tf templates...

I don't remember where either ;). But yeah, I think it's best to leave what-we're-installing naming out internally (where there should be no parallel installers for other things that we need to distinguish ourselves from). We just need openshift-install in the user-facing command.

wking · 2018-11-09T20:48:33Z

data/data/aws/master/main.tf

@@ -96,7 +96,7 @@ resource "aws_instance" "master" {
  tags = "${merge(map(
      "Name", "${var.cluster_name}-master-${count.index}",
      "kubernetes.io/cluster/${var.cluster_name}", "owned",
-      "tectonicClusterID", "${var.cluster_id}",
+      "openshiftClusterID", "${var.cluster_id}",


whoa, boy ;). This one is quasi-public, so I wouldn't be surprised if we break some third-party tooling with this change. Still, I think dropping tectonic makes sense to get consistent branding. And openshift is probably appropriate here, although I'd be open to "kubernetes.io/cluster-id", ${var.cluster_id} if we could drum up an appetite for a cluster UUID tag at the vanilla Kubernetes level.

I have removed the change of tectonicClusterID from this PR. I will do it in a subsequent PR.

wking · 2018-11-09T20:57:10Z

data/data/aws/route53/openshift.tf

@@ -3,17 +3,17 @@ locals {
  private_endpoints_count = "${var.private_endpoints ? 1 : 0}"
 }

-data "aws_route53_zone" "tectonic" {
+data "aws_route53_zone" "openshift" {


Potential generic name here would be base to match base_domain. I'd also be fine with zone or cluster or similar. I'm not terribly opposed to openshift here though, if folks prefer that to my alternatives.

wking · 2018-11-09T21:03:11Z

data/data/bootstrap/systemd/units/openshift.service

+
+[Service]
+WorkingDirectory=/opt/openshift/openshift
+ExecStart=/usr/local/bin/openshift.sh /opt/openshift/auth/kubeconfig


I'd prefer a more specific name for this script and service, since "OpenShift" feels like "Kubernetes plus the OpenShift-specific stuff" and not "just the OpenShift-specific stuff" to me. The script is about pushing our manifests into the cluster, so maybe manifests.sh (and manifests.service)? Or initial-manifests.sh? operator-manifests.sh? Or something? special-sauce-beyond-vanilla-kubernetes.sh? ;)

I had changed it to manifests.service and put the manifests that it used at /opt/openshift/manifests, but there were conflicts with manifests coming from another location. I was not in love with the name manifests.service as there are a lot of different manifests used for different purposes. If we really care about changing this name, then we can have a discussion around better names and change it in another PR.

wking · 2018-11-09T21:08:50Z

data/data/aws/main.tf

-  public_endpoints  = "${var.tectonic_aws_endpoints == "private" ? false : true}"
-  private_zone_id   = "${var.tectonic_aws_external_private_zone != "" ? var.tectonic_aws_external_private_zone : join("", aws_route53_zone.tectonic_int.*.zone_id)}"
+  private_endpoints = "${var.openshift_aws_endpoints == "public" ? false : true}"
+  public_endpoints  = "${var.openshift_aws_endpoints == "private" ? false : true}"


Besides tectonic/openshift being redundant. aws is also redundant (it's implied by the module name). I'd be fine with just ingress or some such.

I agree that aws is redundant. However, the aws is needed by the json marshal/unmarshal to distinguish between fields in AWS, Libvirt, and OpenStack in the pkg/tfvars/config struct in case there are fields that would otherwise have identical names. This is a side effect of inlining all of those fields in pkg/tfvars/config.

This is a side effect of inlining all of those fields in pkg/tfvars/config.

I think Terraform needs a single level. But we only ever need one platform at a time. We could (future work?) invert the structs to embed the generic struct within each per-platform struct.

wking · 2018-11-09T21:10:03Z

This is also pretty big. It might be easier if we try to bite it off in smaller chunks? E.g. just stuff inside Terraform in one PR. Then the Go -> Terraform JSON serialization in a second PR. Then the pure Go stuff in a third PR?

abhinavdahiya · 2018-11-09T21:36:32Z

This is also pretty big. It might be easier if we try to bite it off in smaller chunks? E.g. just stuff inside Terraform in one PR. Then the Go -> Terraform JSON serialization in a second PR. Then the pure Go stuff in a third PR?

i would say just separate commits is fine.. separate PR is too much overhead.

data/data/bootstrap/systemd/units/manifests.service

docs/dev/libvirt-howto.md

abhinavdahiya · 2018-12-01T01:10:03Z

this looks good. @staebler can you do some local libvirt,aws testing and @flaper87 can you take a look to see if any openstack stuff is missing or wrong.

After that lets merge it and iterate then. 😇

pkg/asset/templates/content/bootkube/cvo-overrides.go

wking · 2018-12-05T08:04:24Z

I just launched a libvrit cluster from febbd75, and it came up without issues. And I'm super excited about getting this landed, just needs a rebase now (and one typo fix, although I'm fine landing this without that fix too).

The tectonic prefix in the tfvars are redundant as it identifies the type of installer being used which is evident already by using that installer. Changes for https://jira.coreos.com/browse/CORS-878

This makes the corresponding change in the terraform to match the removal of the terraform prefix in the tfvars json. Changes for https://jira.coreos.com/browse/CORS-878

The session name when connecting to AWS is changed from starting with TECTONIC_INSTALLER_ to starting with OPENSHIFT_INSTALLER. The terraform name of the internal route53 zone is changed from terraform_int to int. The Name tag of the zone is changed from ending with _tectonic_int to ending with _int. The terraform name of the base route53 zone is changed from tectonic to base. The terraform name of the api route53 records are changed from tectonic_api_external and tectonic_api_internal to api_external and api_internal, respectively. Changes for https://jira.coreos.com/browse/CORS-878

The terraform name of the libvirt network is changed from tectonic_net to net. Changes for https://jira.coreos.com/browse/CORS-878

…ules The terraform name of the openstack object storage container is changed from terraform to container. Changes for https://jira.coreos.com/browse/CORS-878

Change the directory of bootstrap files from /opt/tectonic to /opt/openshift. Change the directory of files laid down by the Tectonic Manifests asset from tectonic to openshift. Change the name of the service that lays down OpenShift manifests from tectonic.service to openshift.service. Changes for https://jira.coreos.com/browse/CORS-878

openshift-ci-robot · 2018-12-05T16:44:23Z

@staebler: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	`24ade67`	link	`/test e2e-libvirt`

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.

Change directory for openshift manifests from data/data/manifests/tectonic to data/data/manifests/openshift. Chnages for https://jira.coreos.com/browse/CORS-878

Changes for https://jira.coreos.com/browse/CORS-878

…tonic to manifests/openshift Changes for https://jira.coreos.com/browse/CORS-878

Changes for https://jira.coreos.com/browse/CORS-878

Bazel is no longer used. Changes for https://jira.coreos.com/browse/CORS-878

Changes for https://jira.coreos.com/browse/CORS-878

wking · 2018-12-05T17:18:50Z

/lgtm

openshift-ci-robot · 2018-12-05T17:19:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler, wking

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~~ [staebler,wking]

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

wking · 2018-12-05T17:46:33Z

Looks like the job might be hung in tests? @staebler, can you grant me access to ci-op-rk8i4bxq or poke around there yourself and see if anything looks fishy?

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 Nov 9, 2018

openshift-ci-robot requested review from flaper87 and hardys November 9, 2018 04:40

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2018

wking reviewed Nov 9, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2018

wking reviewed Nov 9, 2018

View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2018

abhinavdahiya reviewed Nov 27, 2018

View reviewed changes

data/data/bootstrap/systemd/units/manifests.service Outdated Show resolved Hide resolved

abhinavdahiya reviewed Nov 27, 2018

View reviewed changes

docs/dev/libvirt-howto.md Outdated Show resolved Hide resolved

staebler changed the title ~~WIP: Replace references to tectonic with openshift~~ Replace references to tectonic with openshift Nov 27, 2018

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 Nov 27, 2018

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 30, 2018

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018

wking mentioned this pull request Dec 4, 2018

libvirt: Add Terraform variables for memory/CPU, bump master to 4GiB #785

Merged

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2018

wking reviewed Dec 5, 2018

View reviewed changes

pkg/asset/templates/content/bootkube/cvo-overrides.go Outdated Show resolved Hide resolved

staebler added 3 commits December 5, 2018 11:14

tfvars: remove tectonic from names of tfvars json

d9212f5

The tectonic prefix in the tfvars are redundant as it identifies the type of installer being used which is evident already by using that installer. Changes for https://jira.coreos.com/browse/CORS-878

terraform: remove tectonic references from terraform config file

e8adc65

This makes the corresponding change in the terraform to match the removal of the terraform prefix in the tfvars json. Changes for https://jira.coreos.com/browse/CORS-878

staebler added 3 commits December 5, 2018 11:14

terraform: remove references to tectonic from libvirt terraform modules

b37ab82

The terraform name of the libvirt network is changed from tectonic_net to net. Changes for https://jira.coreos.com/browse/CORS-878

terraform: remove references to tectonic from openstack terraform mod…

f621260

…ules The terraform name of the openstack object storage container is changed from terraform to container. Changes for https://jira.coreos.com/browse/CORS-878

staebler added 7 commits December 5, 2018 12:09

data/manifests: remove tectonic references from manifests data

bc1508f

Change directory for openshift manifests from data/data/manifests/tectonic to data/data/manifests/openshift. Chnages for https://jira.coreos.com/browse/CORS-878

asset/manifests: rename tectonic manifests asset to openshift manifests

c7809ca

Changes for https://jira.coreos.com/browse/CORS-878

asset/templates: move openshift manifest templates from manifests/tec…

628521b

…tonic to manifests/openshift Changes for https://jira.coreos.com/browse/CORS-878

asset/templates: remove tectonic reference from CVO overrides assets

62bb10e

Changes for https://jira.coreos.com/browse/CORS-878

asset/manifests: remove unusued tectonicCloudProvider function

8d8c437

Changes for https://jira.coreos.com/browse/CORS-878

.: remove unused version.bzl file

16a5a6e

Bazel is no longer used. Changes for https://jira.coreos.com/browse/CORS-878

docs: remove tectonic references from docs

31bd83f

Changes for https://jira.coreos.com/browse/CORS-878

openshift-ci-robot assigned wking Dec 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2018

openshift-merge-robot merged commit 57d9180 into openshift:master Dec 5, 2018

wking mentioned this pull request Dec 11, 2018

*: remove support for environment variables #861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace references to tectonic with openshift #644

Replace references to tectonic with openshift #644

staebler commented Nov 9, 2018 •

edited

Loading

abhinavdahiya commented Nov 9, 2018

wking commented Nov 9, 2018

wking Nov 9, 2018

staebler Nov 30, 2018

wking Nov 9, 2018

wking Nov 9, 2018

staebler Nov 30, 2018

wking Nov 9, 2018

staebler Nov 21, 2018

wking Nov 21, 2018

wking commented Nov 9, 2018

abhinavdahiya commented Nov 9, 2018

abhinavdahiya commented Dec 1, 2018

wking commented Dec 5, 2018

openshift-ci-robot commented Dec 5, 2018 •

edited

Loading

wking commented Dec 5, 2018

openshift-ci-robot commented Dec 5, 2018

wking commented Dec 5, 2018

Replace references to tectonic with openshift #644

Replace references to tectonic with openshift #644

Conversation

staebler commented Nov 9, 2018 • edited Loading

abhinavdahiya commented Nov 9, 2018

wking commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Nov 9, 2018

abhinavdahiya commented Nov 9, 2018

abhinavdahiya commented Dec 1, 2018

wking commented Dec 5, 2018

openshift-ci-robot commented Dec 5, 2018 • edited Loading

wking commented Dec 5, 2018

openshift-ci-robot commented Dec 5, 2018

wking commented Dec 5, 2018

staebler commented Nov 9, 2018 •

edited

Loading

openshift-ci-robot commented Dec 5, 2018 •

edited

Loading