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

Integrate Image Streams #4

Merged
merged 9 commits into from
Mar 6, 2018
Merged

Integrate Image Streams #4

merged 9 commits into from
Mar 6, 2018

Conversation

maneta
Copy link

@maneta maneta commented Feb 19, 2018

ToDo:

  • Check postCommit in the Build (Split resolv.conf in lines APIcast#618)
  • set resource limits/requests
  • Update deployment documentation and place a Makefile
  • Prepare service for prometheus metrics.

Plus separating templates for easier Deployment
@maneta maneta requested review from jmprusi and mikz February 19, 2018 17:36
tags:
- name: latest
annotations:
openshift.io/display-name: Apicast Cloud Hosted (latest)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Apicast/APIcast/ 😉

Copy link
Author

Choose a reason for hiding this comment

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

Oops I did it again :p

app: apicast
spec:
tags:
- name: builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add the branch name to the tag name? So we know it is "master" and not some tag?

Copy link
Author

Choose a reason for hiding this comment

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

Good Idea 👍

imagePullPolicy: IfNotPresent
name: apicast
name: apicast-${RELEASE_REF}
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no need for containers to have a suffix no?

Copy link
Author

Choose a reason for hiding this comment

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

yes sure

image: "${APICAST_IMAGE}"
value: "http://apicast-mapping-service-${RELEASE_REF}/config"
- name: APICAST_OIDC_LOG_LEVEL
value: "${APICAST_OIDC_LOG_LEVEL}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this does not need to be a Parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Ok So I will place the env var with notice as default.

- name: builder
from:
kind: DockerImage
name: quay.io/3scale/apicast:master-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think it is a good idea.

@@ -103,6 +110,13 @@ objects:
- containerPort: 8090
name: management
protocol: TCP
resources:
Copy link
Author

Choose a reason for hiding this comment

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

@mikz Should we set the APICAST_WORKERS too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope. APIcast autodetects number of requested CPU cores: 3scale/APIcast#600

Copy link
Author

Choose a reason for hiding this comment

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

ok

- name: master-builder
from:
kind: DockerImage
name: quay.io/3scale/apicast:master-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have a comment pointing to how is this refreshed.
#4 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

TBH, I don't know how quay.io/3scale/apicast:master-builder is refreshed either... Or are you talking about the scheduled imports from the registry?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Scheduled imports.

Those quay images are built when master branch is updated.

Copy link
Author

Choose a reason for hiding this comment

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

Done in 287e33c

imageChangeParams:
automatic: true
containerNames:
- apicast-${RELEASE_REF}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong.

Copy link
Author

Choose a reason for hiding this comment

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

yep.

maneta added 2 commits February 20, 2018 11:33
The ImageStream will immport the builder imager periodically. This configuration is clusterwide but the default is 15 minutes.
oc new-app -f $(THISDIR_PATH)/04-deployment-template.yml \
-p RELEASE_REF=${RELEASE_REF} \
-p ENVIRONMENT=${ENVIRONMENT} \
-p CACHE_TTL=${CACHE_TTL}
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this crash when CACHE_TTL is empty? It should.

Copy link
Author

Choose a reason for hiding this comment

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

yes it crashes because there is no predefined value in the template.

Actually it brakes without RELEASE_REF AND ENVIRONMENT also.

The actuall error message:

oc new-app -f /Users/dcesario/src/apicast-cloud-hosted/openshift/04-deployment-template.yml \
		-p RELEASE_REF=test-dani3 \
		-p ENVIRONMENT=staging \
		-p CACHE_TTL=
error: error processing template "apicast-staging/apicast-cloud-hosted-deployment": Template "apicast-cloud-hosted-deployment" is invalid: template.parameters[2]: Required value: template.parameters[2]: parameter CACHE_TTL is required and must be specified
make: *** [deploy] Error 1

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@maneta
Copy link
Author

maneta commented Feb 20, 2018

@mikz Should we investigate how to build the apicast-mapping-service image or should we let it for the future (as an issue)?

- '--daemon'
command:
- bin/apicast
#postCommit:
Copy link
Author

Choose a reason for hiding this comment

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

@mikz Should we remove the postCommit ? The Deployment is working without it, but I'm not sure how to solve it...

Copy link
Contributor

Choose a reason for hiding this comment

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

postCommit is good. It verifies the image will boot. Otherwise you have no idea until you try to deploy it (and fail).

Copy link
Author

@maneta maneta Feb 20, 2018

Choose a reason for hiding this comment

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

Ok you said something about Apicasy and the RESOLVER env Variable I will take a look at it

Copy link
Author

@maneta maneta Feb 21, 2018

Choose a reason for hiding this comment

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

@mikz So the problem with the postCommit is that the openshift builder is setting the resolv.conf file like that:

# nameserver updated by /etc/NetworkManager/dispatcher.d/99-origin-dns.sh
# Generated by NetworkManager
search ec2.internal cluster.local
nameserver 10.0.101.97

The problem is the first line, looks like the parse_nameservers function in APIcast https://github.com/3scale/apicast/blob/5c2dc6bbf9295b5e7891fb7bb52e32abcf763972/gateway/src/resty/resolver.lua#L105 is matching with the commented nameserver in the file.

The actual error thrown by nginx is:

nginx: [emerg] invalid IPv6 address in resolver "[domain]:53" in /tmp/lua_c5D5o5:45

I have reproduced it locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Looks like: https://github.com/3scale/apicast/blob/5c2dc6bbf9295b5e7891fb7bb52e32abcf763972/gateway/src/resty/resolver.lua#L135-L140

This should match only from start of the line and ignore comments. We probably need better parsing anyway.

Good catch!

Copy link
Author

Choose a reason for hiding this comment

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

Corrected in 3scale/APIcast#618

@maneta maneta changed the title WIP - Integrate Image Streams Integrate Image Streams Feb 28, 2018
@maneta
Copy link
Author

maneta commented Feb 28, 2018

@mikz please review 58d7958 when you can.

@maneta maneta merged commit 071ba51 into master Mar 6, 2018
@maneta maneta deleted the introduce-imagestreams branch March 6, 2018 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants