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

Add log watch entry points #719

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

akiselev1
Copy link
Member

@akiselev1 akiselev1 commented Nov 13, 2020

Adding ironic and ironic-inspector log watch entry points per this design proposal: https://github.com/metal3-io/metal3-docs/blob/master/design/ironic-debuggability-improvement.md

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 13, 2020
@akiselev1
Copy link
Member Author

/cc stbenjam

@akiselev1 akiselev1 changed the title Add log watch entrypoints Add log watch entry points Nov 13, 2020
@dhellmann
Copy link
Member

/cc @kashifest @maelk

/test-integration

@stbenjam
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akiselev1, stbenjam

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2020
@dhellmann
Copy link
Member

/test-integration
/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2020
@dhellmann
Copy link
Member

/approve
/lgtm cancel

Oops, I meant to give @maelk and @kashifest a chance to review this.

@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2020
@kashifest
Copy link
Member

I am not sure if we would want to add the container here also https://github.com/metal3-io/baremetal-operator/blob/master/tools/run_local_ironic.sh since in case of test integration we deploy the ironic locally. When this is decided, I would also like to run centos tests to verify ironic deployment.

@maelk
Copy link
Member

maelk commented Nov 16, 2020

+1 to adding it to the run_local_ironic.sh script. Otherwise lgtm

@maelk
Copy link
Member

maelk commented Nov 16, 2020

btw, if it is added to run_local_ironic.sh, the cleanup scripts in metal3-dev-env will need to be updated too.

@akiselev1
Copy link
Member Author

btw, if it is added to run_local_ironic.sh, the cleanup scripts in metal3-dev-env will need to be updated too.

@maelk Can you share the link to these cleanup scripts?

@akiselev1
Copy link
Member Author

I am not sure if we would want to add the container here also https://github.com/metal3-io/baremetal-operator/blob/master/tools/run_local_ironic.sh since in case of test integration we deploy the ironic locally. When this is decided, I would also like to run centos tests to verify ironic deployment.

@kashifest What is the command to run centos tests?

@maelk
Copy link
Member

maelk commented Nov 16, 2020

https://github.com/metal3-io/metal3-dev-env/blob/master/lib/common.sh#L426

The command would be / test-centos-integration

@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 16, 2020
@akiselev1
Copy link
Member Author

/test-centos-integration

@akiselev1
Copy link
Member Author

/test-integration

1 similar comment
@akiselev1
Copy link
Member Author

/test-integration

@akiselev1
Copy link
Member Author

/test-integration

@fmuyassarov
Copy link
Member

I'm not 100% sure if that's the reason for the below issue on the failed CI

+ sudo docker run -d --net host --privileged --name ironic-log-watch --entrypoint /bin/runlogwatch.sh -v /opt/metal3-dev-env/ironic:/shared 192.168.111.1:5000/localimages/ironic
4e7fd9e40796358daacc433d3bbd983c745f2da64b79ac6028f10fb1ff01226a
docker: Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "exec: \"/bin/runlogwatch.sh\": permission denied": unknown.
make: *** [Makefile:10: launch_mgmt_cluster] Error 126

but it seems that runlogwatch.sh is not executable (no +x permission).
I ran this container locally, and went inside and I saw that runlogwatch.sh doesn't have +x permission. See below

fmuyassarov@est:~$ docker ps
CONTAINER ID        IMAGE                      COMMAND                  CREATED             STATUS              PORTS               NAMES
72f75fae68a9        quay.io/metal3-io/ironic   "/bin/runironic --en…"   12 seconds ago      Up 11 seconds                           nervous_shtern
fmuyassarov@est:~$ docker exec -it 72 /bin/bash
[root@72f75fae68a9 /]# ls -lah /bin/ | grep runlogwatch.sh
-rw-r--r-- 1 root root   389 Nov 12 10:57 runlogwatch.sh
[root@72f75fae68a9 /]# 

@akiselev1
Copy link
Member Author

akiselev1 commented Nov 17, 2020

@fmuyassarov Thank you! I think I know what happened. I forgot to set +x permission in the original PR, and created a new one: metal3-io/ironic-image#221 to fix that. You can see correct +x permission if you do git clone https://github.com/metal3-io/ironic-image.git. Apparently that did not re-build the image in Quay (why?). I guess another ironic-image PR with some minor change in Dockerfile would push this change to Quay. Is there any better/smarter way to re-build the container and push it to Quay? cc: @maelk , @dhellmann

@fmuyassarov
Copy link
Member

fmuyassarov commented Nov 17, 2020

Make sense. Last built in the quay for the Ironic image was on Noveber 12th, 13:02 (I'm not sure though if it shows my local time)

quay

and from the git log your fix was merged on November 12th, at 14:02 (it is my local time). Maybe there is some time mismatch on my browser. I would assume quay was triggered, but then ...why didn't it take changes...

Merge: 749dec4 47ec04f
Author: metal3-io-bot <55852648+metal3-io-bot@users.noreply.github.com>
Date:   Thu Nov 12 14:02:08 2020 -0500

    Merge pull request #221 from akiselev1/fix-script-permissions
    
    Add runlogwatch.sh +x permission

But hopefully Doug or Maël have some ideas. Maybe there is a way we can trigger the build for the quay?

@akiselev1
Copy link
Member Author

Looks like we hit an iceberg: You have reached your pull rate limit.

@akiselev1
Copy link
Member Author

/test-integration

@fmuyassarov
Copy link
Member

Something went wrong in the CI infra I believe.
/test-integration

@akiselev1
Copy link
Member Author

Hm... Ironic image did get updated in Quay. But CI results are empty. Do we have any doc on metal3 CI architecture? Let me know if I can help in any way.

@kashifest
Copy link
Member

it seems like the opensuse pages are down. The CI failed to download the release keys.

@akiselev1
Copy link
Member Author

Opensuse is back.
/test-integration

@akiselev1
Copy link
Member Author

/cc @fmuyassarov @maelk @kashifest - CI finally passed. May I have lgtm from one of you?

@kashifest
Copy link
Member

/lgtm
/test-v1a3-integration
/test-v1a3-centos-integration
Just to be absolutely sure, will run one more round of CI with v1a3

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2020
@maelk
Copy link
Member

maelk commented Nov 19, 2020

/test-v1a3-centos-integration

@metal3-io-bot metal3-io-bot merged commit 906601c into metal3-io:master Nov 19, 2020
@akiselev1 akiselev1 deleted the add-logwatch-entrypoints branch November 19, 2020 16:30
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants