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

Populate the memory limits of functions in the REST API #206

Merged
merged 6 commits into from
Nov 1, 2021

Conversation

Shikachuu
Copy link
Contributor

@Shikachuu Shikachuu commented Sep 18, 2021

Signed-off-by: Shikachuu [email protected]

Description

Add memory limits to the response json file, if you query functions through the API.

Motivation and Context

How Has This Been Tested?

  1. take an existing faasd deployment (a vm for example)
  2. stop the faasd and the faasd-provider service with systemctl
    sudo systemctl stop faasd faasd-provider
  3. copile the faasd binary from my fork (in my case i used the amd64 arch)
    make dist
  4. replace /usr/local/bin/faasd with the one that we compiled in the previous step
    sudo mv bin/faasd /usr/local/bin/faasd
  5. chown /usr/local/bin/faasd to root
    sudo chown root /usr/local/bin/faasd
  6. start the faasd and the faasd-provider services with systemctl
    sudo systemctl start faasd faasd-provider
  7. get your faasd API password
    FAASDPW=$(sudo cat /var/lib/faasd/secrets/basic-auth-password ; echo)
  8. deploy the following function with faas-cli deploy -f ./fn.yml
verison: 1.0
provider:
  name: openfaas
  gateway: http://localhost:8080
functions:
  figlet:
    skip_build: true
    image: ghcr.io/openfaas/figlet:latest
    limits:
      memory: 20M
  1. do a get request with curl to the gateway API and pipe it to jq to see the result
    curl -s http://admin:$FAASDPW@localhost:8080/system/functions | jq
    Example output:
{
  "name": "figlet",
  "image": "ghcr.io/openfaas/figlet:latest",
  "namespace": "openfaas-fn",
  "envProcess": "figlet",
  "envVars": {
    "write_debug": "false"
  },
  "labels": {},
  "annotations": {},
  "limits": {
    "memory": "20M"
  },
  "invocationCount": 1,
  "replicas": 1,
  "createdAt": "2021-09-20T20:03:18.117439332Z"
}

Or you can query just for the limits object: curl -s http://admin:$FAASDPW@localhost:8080/system/functions | jq .[0].limits
Example output:

{
  "memory": "20M"
}

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Commits:

  • I've read the CONTRIBUTION guide
  • My commit message has a body and describe how this was tested and why it is required.
  • I have signed-off my commits with git commit -s for the Developer Certificate of Origin (DCO)

Code:

  • My code follows the code style of this project.
  • I have added tests to cover my changes.

Docs:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Shikachuu added 2 commits September 18, 2021 00:42
…n to k8s resource value and return it through the REST API.

Signed-off-by: Shikachuu <[email protected]>
@alexellis
Copy link
Member

In the testing instructions, can you give us a YAML file or equivalent that will deploy a function with memory limits?

@alexellis
Copy link
Member

alexellis commented Sep 20, 2021

I've left some comments for you and would like both the input for the scenario and the output in step 8 to be added to show what came back.

Here's a potential input you could use:

functions:
  figlet:
    skip_build: true
    image: ghcr.io/openfaas/figlet:latest
    limits:
      memory: 20Mi

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

I've left a few comments, but this is shaping up nicely.

@Shikachuu Shikachuu requested a review from alexellis September 20, 2021 19:46
@Shikachuu
Copy link
Contributor Author

I've left some comments for you and would like both the input for the scenario and the output in step 8 to be added to show what came back.

Here's a potential input you could use:

functions:
  figlet:
    skip_build: true
    image: ghcr.io/openfaas/figlet:latest
    limits:
      memory: 20Mi

Duly noted, PR updated with example function + example responses!

@alexellis
Copy link
Member

If you still have the environment running, can you test with Mi units?

My example with figlet uses Mi and yours uses M.

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

👏 Great Pull Request and testing. Thank you.

I've left one comment for an end to end test scenario I'd like you to run before I merge this.

@Shikachuu Shikachuu requested a review from alexellis September 22, 2021 18:12
@alexellis
Copy link
Member

@Shikachuu would you like to comment on or resolve the conversations above?

@Shikachuu
Copy link
Contributor Author

No, thanks, I think I am a-okay with this implementation.

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

LGTM

@alexellis
Copy link
Member

#206 (comment)

I'm sorry that I was vague. That was me asking you to review and action the feedback. I'll try to be more direct in the future.

@alexellis alexellis merged commit b42066d into openfaas:master Nov 1, 2021
@alexellis
Copy link
Member

@nitishkumar71 @LucasRoesler can we work this functionality into the tests of the certifier?

@nitishkumar71
Copy link
Member

@nitishkumar71 @LucasRoesler can we work this functionality into the tests of the certifier?

Sure, I will work on it.

@Shikachuu Shikachuu deleted the populate_memory_limits branch December 18, 2021 17:11
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.

3 participants