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

Bundle lib 0.2 #946

Merged
merged 7 commits into from Jun 6, 2018
Merged

Bundle lib 0.2 #946

merged 7 commits into from Jun 6, 2018

Conversation

shawn-hurley
Copy link
Contributor

Describe what this PR does and why we need it:

Bump bundle lib for next release

Changes proposed in this pull request

  • re-adding origin copy back
  • rename apb -> bundle package

Does this PR depend on another PR (Use this to track when PRs should be merged)

This depends on a release branch being created and master to open up for new development.

@shawn-hurley shawn-hurley added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 11, 2018
rules := make([]rbac.PolicyRule, 0, len(in))
for _, rule := range in {
// We need to split this rule into multiple rules for RBAC
if isResourceRule(&rule) && isNonResourceRule(&rule) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This reads a bit strange but I can understand what it is doing. I had to read the definitions of the functions to get it though

Copy link
Contributor

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

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

Change looks good to me

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

Approved pending release branch.

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

forgot to hit approve

@maleck13
Copy link
Contributor

maleck13 commented May 15, 2018

@shawn-hurley seeing an issue on this branch with the watchBundle:


panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x11255d5]

goroutine 1396 [running]:
github.com/openshift/ansible-service-broker/vendor/github.com/automationbroker/bundle-lib/runtime.provider.WatchRunningBundle(0x1a726a0, 0x26c2978, 0x1a88120, 0x26c2978, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/kelly/projects/src/github.com/openshift/ansible-service-broker/vendor/github.com/automationbroker/bundle-lib/runtime/runtime.go:392 +0x65
github.com/openshift/ansible-service-broker/vendor/github.com/automationbroker/bundle-lib/bundle.(*executor).Deprovision.func1(0xc4209b0280, 0xc4204432c0)
	/Users/kelly/projects/src/github.com/openshift/ansible-service-broker/vendor/github.com/automationbroker/bundle-lib/bundle/deprovision.go:66 +0x20e
created by github.com/openshift/ansible-service-broker/vendor/github.com/automationbroker/bundle-lib/bundle.(*executor).Deprovision
/Users/kelly/projects/src/github.com/openshift/ansible-service-broker/vendor/github.com/automationbroker/bundle-lib/bundle/deprovision.go:36 +0x2d4 

Investigating further. Looks like the WatchRunningBundle method is nil and never set to the defaultWatchRunningBundle I changed my local code to set this as the default

if config.WatchBundle == nil{
		p.watchBundle = defaultWatchRunningBundle
	}

Which seemed to solve the problem

@maleck13
Copy link
Contributor

maleck13 commented May 15, 2018

Not sure if these are related to my local environment but also seeing the following error during provision of the hello world database apb from the ansibleplaybookbundle org using OpenShift 3.9

[2018-05-15T10:40:07.276Z] [DEBUG] - service_id: b43a4272a6efcaaa3e0b9616324f1099
[2018-05-15T10:40:07.276Z] [DEBUG] - plan_id: db1cdb40646cd408a924445e2b95a1bf
[2018-05-15T10:40:07.276Z] [DEBUG] - operation:  68c9c8a9-cfb0-4b3f-8ad6-a3676ad57187
[2018-05-15T10:40:07.283Z] [DEBUG] - state: in progress
[2018-05-15T10:40:07.283Z] [DEBUG] - description: action started
172.17.0.4 - - [15/May/2018:10:40:07 +0000] "GET /ansible-service-broker/v2/service_instances/275e125a-6aba-41a1-bda5-f9a2350772b9/last_operation?operation=68c9c8a9-cfb0-4b3f-8ad6-a3676ad57187&plan_id=db1cdb40646cd408a924445e2b95a1bf&service_id=b43a4272a6efcaaa3e0b9616324f1099 HTTP/1.1" 200 64
time="2018-05-15T10:40:07Z" level=info msg="command output: rpc error: code = 13 desc = invalid header field value \"oci runtime error: exec failed: container_linux.go:247: starting container process caused \\\"exec: \\\\\\\"broker-bind-creds\\\\\\\": executable file not found in $PATH\\\"\\n\"\r\n - err: "
time="2018-05-15T10:40:07Z" level=info msg="retry attempt: 13 pod: apb-f692c315-281d-4feb-96e4-ebf7e3849531 in namespace: dh-hello-world-db-apb-prov-xblqw failed to exec into the container"
time="2018-05-15T10:40:12Z" level=info msg="command output:  - err: "
time="2018-05-15T10:40:12Z" level=info msg="retry attempt: 14 pod: apb-f692c315-281d-4feb-96e4-ebf7e3849531 in namespace: dh-hello-world-db-apb-prov-xblqw failed to exec into the container"
time="2018-05-15T10:40:17Z" level=info msg="pod: apb-f692c315-281d-4feb-96e4-ebf7e3849531 in namespace: dh-hello-world-db-apb-prov-xblqw has been completed"
time="2018-05-15T10:40:17Z" level=error msg="apb::provision error occurred - unexpected end of JSON input"
time="2018-05-15T10:40:17Z" level=info msg="Destroying APB sandbox..."

@shawn-hurley
Copy link
Contributor Author

@maleck13 any chance you can submit the patch to bundle lib?

@rthallisey rthallisey added the 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 label May 15, 2018
@shawn-hurley shawn-hurley added 3.11 | release-1.3 Kubernetes 1.11 | Openshift 3.11 | Broker release-1.3 and removed 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 labels May 15, 2018
@maleck13
Copy link
Contributor

maleck13 commented May 16, 2018

@shawn-hurley For the second issue it looks to me like there is an attempt to exec into the container to get the credentials failed to exec into the container . Not sure why it is failing yet digging in

@maleck13
Copy link
Contributor

@shawn-hurley @mhrivnak have a fix on the way for the second issue around the runtime label (discussed on IRC yesterday). Taking the approach of defaulting to using the previous label

com.redhat.apb.runtime

but overriding with

com.redhat.bundle.runtime

if it is present.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2018
@shawn-hurley shawn-hurley removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2018
)

//ConvertAPIPolicyRulesToRBACPolicyRules - Convert Policy Rules.
func ConvertAPIPolicyRulesToRBACPolicyRules(in []v1.PolicyRule) []rbac.PolicyRule {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍, happy if we can rip this out of bundle-lib.

@jmrodri
Copy link
Contributor

jmrodri commented May 22, 2018

I retested this and things worked fine. Let me know if we should merge.

@maleck13
Copy link
Contributor

Something looks to have changed with the state work mount looks like this

Mount: 6a86256e-d766-4144-af44-7ebecef16f46-state → /etc/apb/state 

but the actual config map is now copied over as

bundle-50f5623c-f6bc-4278-adf7-f590ae12b72c

investigating

@maleck13
Copy link
Contributor

maleck13 commented May 22, 2018

The issue here is the mount is using the configmap name from the broker ns. This is set on the execution context as StateName but actually the configmap in the temporary ns is named after the pod. So I think the following needs to change
https://github.com/automationbroker/bundle-lib/blob/master/runtime/run_bundle.go#L64
to use BundleName

Also seem to have lost the pod env var for the state location for the module to use https://github.com/ansibleplaybookbundle/ansible-asb-modules/pull/15/files#diff-626ec7d8b94f76ad88ac6d856a17b660R34
I will create a PR

@jmrodri
Copy link
Contributor

jmrodri commented Jun 4, 2018

rebased to pick up the travis changes from @djzager

@shawn-hurley shawn-hurley merged commit 293ec7d into master Jun 6, 2018
@jmrodri jmrodri deleted the bundle-lib-0.2 branch June 7, 2018 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 | release-1.3 Kubernetes 1.11 | Openshift 3.11 | Broker release-1.3 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants