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.

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

sharded-test-server: support standalone vw server #1800

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Aug 19, 2022

Summary

When running sharded-test-server, if you now pass --shard-run-virtual-workspaces=false, it will run virtual-workspaces in
standalone mode, and configure the front proxy to map to the virtual-workspaces process for /services.

Note, this does not currently wire anything into CI/e2e tests. Figured we'd want to discuss where to plumb this in.

Related issue(s)

Fixes #1702

@@ -53,9 +60,9 @@ func startFrontProxy(ctx context.Context, args []string, servingCA *crypto.CA, h
lineprefix.Color(color.New(color.FgHiWhite)),
)

if err := ioutil.WriteFile(filepath.Join(workDirPath, ".kcp-front-proxy/mapping.yaml"), []byte(`
if err := ioutil.WriteFile(filepath.Join(workDirPath, ".kcp-front-proxy/mapping.yaml"), []byte(fmt.Sprintf(`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're now print-formatting some YAML, would it be difficult to define an anonymous type here and marshal it instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will look into it

commandLine = append(
commandLine,
"--kubeconfig=.kcp-0/admin.kubeconfig",
"--context=system:admin",
Copy link
Member

Choose a reason for hiding this comment

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

@s-urbaniak @csams @kylape reminder: we definitely don't want this medium term, but find a way, probably some special RBAC permission to do wildcard list/watch.

ncdc added 3 commits August 29, 2022 16:27
Add --context to the standalone virtual-workspaces process to allow
specifying the context in the kubeconfig to use.

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
When running sharded-test-server, if you now pass
--shard-run-virtual-workspaces=false, it will run virtual-workspaces in
standalone mode, and configure the front proxy to map to the
virtual-workspaces process for /services.

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
@ncdc ncdc force-pushed the e2e/standalone-virtual branch from 1555b35 to 32eab91 Compare August 29, 2022 21:27
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
ProxyClientCert: ".kcp-front-proxy/requestheader.crt",
ProxyClientKey: ".kcp-front-proxy/requestheader.key",
},
}
Copy link
Member

Choose a reason for hiding this comment

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

for follow-up: we should really turn this mapping file into flags. The file promises flexibility that is not there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an issue for that?

Copy link
Member

Choose a reason for hiding this comment

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

Now there is: #1857


func startVirtual(ctx context.Context, index int, logDirPath string) (<-chan error, error) {
logger := klog.FromContext(ctx)
logger.Info("starting virtual-workspaces standalone server", "index", index)
Copy link
Member

Choose a reason for hiding this comment

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

nit: not sure we do that elsewhere, but maybe print the command line with all parameters instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sttts
Copy link
Member

sttts commented Aug 30, 2022

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit c107a10 into kcp-dev:main Aug 30, 2022
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
4 participants