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

CPU_PERCENT and GPS examples do not expose port #555

Closed
johnwalicki opened this issue Sep 13, 2022 · 12 comments · Fixed by #580
Closed

CPU_PERCENT and GPS examples do not expose port #555

johnwalicki opened this issue Sep 13, 2022 · 12 comments · Fixed by #580

Comments

@johnwalicki
Copy link
Member

Describe the bug
The CPU Percent example does not expose the port to query the CPU load
See https://github.com/open-horizon/examples/blob/master/edge/services/cpu_percent/horizon/service.definition.json

The README shows a curl excample but the user would need to run the curl within the workload container.
I am not certain that is the expected behavior.
The Makefile has a test rule which calls

../../../tools/curlServiceTest.sh

which is here: https://github.com/open-horizon/examples/blob/master/tools/curlServiceTest.sh
That script does a docker exec into the container and curl works.

If the purpose is to not expose the CPU performance data, then clarify in the README
If the intended example should report the CPU data, then service.definition.json should expose the port.

@tbsloan
Copy link

tbsloan commented Sep 13, 2022

Hi John,
I will work with Troy, I think he owns/maintains those examples.

@tbsloan
Copy link

tbsloan commented Sep 13, 2022

John - I contacted Troy - he was aware from examples meeting where this was discussed:

Troy Fine
1:49 PM
Hey Tim, ah yes John brought that up in the examples working group meeting this morning and mentioned he would open the issue … I’ll take a look at some of the documentation around and try to decide whether to make the change to expose the port or simply update the readme to mention you would need to exec into the container first

@t-fine t-fine self-assigned this Sep 13, 2022
@johnwalicki
Copy link
Member Author

@tbsloan Oops, sorry for the misdirect. GitHub type-ahead tagged you instead of Troy.

@tbsloan
Copy link

tbsloan commented Sep 13, 2022

@johnwalicki No worries, we got it moved over to Troy.

@t-fine
Copy link
Collaborator

t-fine commented Oct 11, 2022

Similar to the open-horizon-services web-helloworld-python service exposes port 8000 we would like cpu_percent and gps to also expose the port to allow the service to be curl-ed from outside the container.

web-helloworld-python service.json file: https://github.com/open-horizon-services/web-helloworld-python/blob/main/service.json

cpu_percent: https://github.com/open-horizon/examples/tree/master/edge/services/cpu_percent

gps: https://github.com/open-horizon/examples/tree/master/edge/services/gps

@t-fine t-fine changed the title CPU_PERCENT example does not expose port CPU_PERCENT and GPS examples do not expose port Oct 11, 2022
@saurav1004
Copy link
Contributor

@t-fine Could I be assigned this issue ?

If I am not wrong we need to expose the port in cpu_percent/horizon/service.defination.json and gps/horizon/service.defination.json so that the service can be curl-ed from outside the container.

@johnwalicki
Copy link
Member Author

@saurav1004 - Yes, the service.definition.json files would expose the port. The web-helloworld-python service json has a snippet. Check the port number, it might be 8080

        "ports": [ 
          { "HostIP": "0.0.0.0", "HostPort": "8080:8080/tcp" }
        ]

@saurav1004
Copy link
Contributor

Yes the port number is 8080 for both the services cpu_percent and gps

@saurav1004
Copy link
Contributor

@johnwalicki I have created a draft pull request, will mark it "ready for review" once you confirm.

@johnwalicki
Copy link
Member Author

The PR looks good. I think it's ready to merge.

@saurav1004
Copy link
Contributor

Thanks, I have marked it ready for review. You should be able to merge it now.

@t-fine
Copy link
Collaborator

t-fine commented Feb 28, 2023

With the bump of version number of cpu_percent and gps in the PR just merged (#580) I will be closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants