Skip to content

Implement index server #12

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

Merged
merged 6 commits into from
Dec 2, 2020

Conversation

GeekArthur
Copy link
Contributor

Signed-off-by: jingfu wang [email protected]

Please specify the area for this PR

What does does this PR do / why we need it:
This PR implements the new index server, it also integrates with nginx server to serve non-registry API requests.

Which issue(s) this PR fixes:

Fixes devfile/api#188

PR acceptance criteria:

  • Test (WIP)

  • Documentation (WIP)

How to test changes / Special notes to the reviewer:
Deploy index server and registry as before, send GET request to endpoint /index or /devfiles/{devfile name}

Signed-off-by: jingfu wang <[email protected]>
@GeekArthur GeekArthur self-assigned this Nov 23, 2020
@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 23, 2020
Signed-off-by: jingfu wang <[email protected]>
Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

Left some review comments.

Another suggestion I had is that since this is called index-server, does it make sense to move it under the index/ folder in this repo?

e.g.

index/
   generator/
   server/

@GeekArthur
Copy link
Contributor Author

Left some review comments.

Another suggestion I had is that since this is called index-server, does it make sense to move it under the index/ folder in this repo?

e.g.

index/
   generator/
   server/

I though about the same thing before, will do that.

Signed-off-by: jingfu wang <[email protected]>
@GeekArthur
Copy link
Contributor Author

@johnmcollier comments addressed, please review again, thanks!

Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

Every time the index server starts for the first time, it exits with an error:

^CJohns-MacBook-Pro-3:kubernetes johncollier$ oc get po
NAME                                                   READY   STATUS    RESTARTS   AGE
devfile-registry-8467d84c6d-zrwgw                      2/2     Running   1          4m20s
registry-operator-controller-manager-9b6b557b4-79hpz   2/2     Running   0          15m

I think it's happening because of the log.Fatal we do if the http.Get to the registry server returns an error. Probably the registry server isn't quite ready yet and isn't giving back an http response code?)

I think it might be better if we don't exit when an http.Get returns an error, but rather keep trying up to a given timeout?

This is what I did in the registry operator when I needed to poll a given URL (see https://github.com/johnmcollier/registry-operator/blob/main/pkg/util/util.go#L25)

func WaitForServer(url string, timeout time.Duration) error {
	return wait.PollImmediate(time.Second, timeout, func() (bool, error) {
		tr := &http.Transport{
			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
		}
		client := &http.Client{Transport: tr}
		resp, err := client.Get(url)
		if err != nil {
			return false, err
		}
		if resp.StatusCode == 200 {
			return true, nil
		}
		return false, nil
	})
}

I'd recommend setting the timeout to no more than 30 seconds, since any more and the Kube liveness probe will fail and kill the container. You may also want to change the time.Second in wait.PollImmediate to some multiple of time.Millisecond.

@GeekArthur
Copy link
Contributor Author

@johnmcollier Can you help review again? Thanks

@GeekArthur
Copy link
Contributor Author

@johnmcollier changes are pushed, please review.

Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GeekArthur, johnmcollier

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 [GeekArthur,johnmcollier]

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

@GeekArthur GeekArthur merged commit 0443f20 into devfile:master Dec 2, 2020
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.

Re-write the OCI registry metadata container in Go
3 participants