Include Test case for Limits #83

nitishkumar71 · 2021-11-05T04:41:43Z

Signed-off-by: Nitishkumar Singh [email protected]

Certifier should support validation of memory limit in faasd and openfaas

Description

Motivation and Context

I have raised an issue to propose this change this is required

Closes #81

How Has This Been Tested?

New test case has been included to test memory limit

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 describes 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.

nitishkumar71 · 2021-11-05T09:19:01Z

Can we run the workflow again? The issue is same as #82 where mostly failure happens for logs.

alexellis · 2021-11-10T16:58:59Z

tests/deploy_test.go

+	} else if config.ProviderName == faasdProviderName && deploy.Limits != nil && status.Limits != nil {
+		deployLimits := *deploy.Limits
+		statusLimits := *status.Limits
+		if deployLimits.Memory != statusLimits.Memory {


I would like to see this fail for the current version of faasd which has no memory limit support in its status endpoint.

Changes are done. Since faasd does not support cpu, special handling is required for faasd.

Changes are done to allow current stable faasd version to fail for Limits

alexellis · 2021-12-01T16:47:36Z

You can run this locally with go test -run "^NameOfTest$" and then see the individual test fail for faasd to show that the memory limit wasn't detected. Then we'll cut a new release of faasd we can re-run the tests and see it pass.

alexellis · 2021-12-08T16:17:46Z

2021-12-05T17:50:48.3391383Z --- FAIL: Test_Deploy_MetaData/test_listing_functions (0.02s)
2021-12-05T17:50:48.3392022Z deploy_test.go:162: got nil, expected Limits &{5M 100m}

Perfect!

alexellis · 2021-12-08T16:19:50Z

We now have a new release which populates the memory value in the API, so let's run these tests again once the binaries are ready.

After that, if the faasd tests pass, I'll merge this PR.

https://github.com/openfaas/faasd/releases/tag/0.14.4

nitishkumar71 · 2021-12-12T10:50:51Z

We now have a new release which populates the memory value in the API, so let's run these tests again once the binaries are ready.

After that, if the faasd tests pass, I'll merge this PR.

https://github.com/openfaas/faasd/releases/tag/0.14.4

Can we run it now? I don't have permission for same.

nitishkumar71 · 2021-12-24T05:59:33Z

@alexellis @LucasRoesler I have removed faasd memory test for now and have kept it just for faas-netes. Will work on it once we have resolved openfaas/faasd#228

alexellis · 2022-01-05T16:59:55Z

tests/deploy_test.go

+				Labels:      &map[string]string{},
+				Namespace:   config.DefaultNamespace,
+				Limits: &types.FunctionResources{
+					Memory: "5M",


Could you try this with 5Mi?

Changes are done and it works for both faas-netes and faasd.

LucasRoesler

@nitishkumar71 i added a comment about improving the readability of the test by using a new config flag instead of the provider name.

Another approach could be to have a function SupportsCPULimits(name string) bool that takes the provider name and returns true if it is in the supported list. But either way, using the provider name in the control flow is going to be a bit fragile in the long term.

LucasRoesler · 2022-01-06T08:59:11Z

tests/deploy_test.go

+	if config.ProviderName != faasdProviderName && !reflect.DeepEqual(deploy.Limits, status.Limits) {
 		return fmt.Errorf("got %v, expected Limits %v", status.Limits, deploy.Limits)
+	} else if config.ProviderName == faasdProviderName && deploy.Limits != nil {
+		if status.Limits == nil {
+			return fmt.Errorf("got nil, expected Limits %v", deploy.Limits)
+		}
+		if deploy.Limits.Memory != status.Limits.Memory {
+			return fmt.Errorf("got %s, expected Requested Limit %s", status.Limits.Memory, deploy.Limits.Memory)
+		}


If i am reading this correctly, this implies that faasd does not record and therfore can not return the CPU limits from the original function deployment, is that correct?

I think we can make this semantically clearer and easier to maintain by adding a new boolean value to the config config.SupportCPULimits. When the test starts, we can set config.SupportCPULimits = config.ProviderName != faasdProviderName in the main_test.go

I would then split this into two tests, one for memory and one for CPU

Suggested change

if config.ProviderName != faasdProviderName && !reflect.DeepEqual(deploy.Limits, status.Limits) {

return fmt.Errorf("got %v, expected Limits %v", status.Limits, deploy.Limits)

} else if config.ProviderName == faasdProviderName && deploy.Limits != nil {

if status.Limits == nil {

return fmt.Errorf("got nil, expected Limits %v", deploy.Limits)

}

if deploy.Limits.Memory != status.Limits.Memory {

return fmt.Errorf("got %s, expected Requested Limit %s", status.Limits.Memory, deploy.Limits.Memory)

}

if deploy.Limits != nil {

if status.Limits == nil {

return fmt.Errorf("got nil, expected Limits %v", deploy.Limits)

}

if deploy.Limits.Memory != status.Limits.Memory {

return fmt.Errorf("got %s, expected Requested Limit %s", status.Limits.Memory, deploy.Limits.Memory)

}

if config.SupportCPULimits && deploy.Limits.CPU != status.Limits.CPU {

return fmt.Errorf("got %s, expected Requested Limit %s", status.Limits.CPU, deploy.Limits.CPU)

}

}

if deploy.Limits == nil && status.Limits != nil {

return fmt.Errorf("got %s, expected nil", status.Limits)

}

I think using a config flag here makes the intent of the test much clearer and generic.

In general, I prefer the tests to mention features not providers.

Thank you for suggestion. No doubt this is the best way to write the code. There is an issue in faasd due to which below lines will not work for functions for which no memory limit is explicitly set.

if deploy.Limits == nil && status.Limits != nil { return fmt.Errorf("got %s, expected nil", status.Limits) }

These changes will work only after merge and release of openfaas/faasd#232. I have validated these changes in my local.

@nitishkumar71 now that openfaas/faasd#232 is merged, what do we need to do to merge this?

I think you need to force push again to trigger the actions to re-run the tests

Signed-off-by: Nitishkumar Singh <[email protected]> Test case changed to binary SI from decimal SI Signed-off-by: Nitishkumar Singh <[email protected]> Included Suggestions Signed-off-by: Nitishkumar Singh <[email protected]> Prefixed Docker registery path Signed-off-by: Nitishkumar Singh <[email protected]> revrted wait part Signed-off-by: Nitishkumar Singh <[email protected]>

LucasRoesler · 2022-04-06T09:01:44Z

I am going to merge this because the test that is failing is the scaling test, which has been flaky and we are still investigating. The new limits test is fine

nitishkumar71 force-pushed the memory_support branch 4 times, most recently from 0e55657 to 60bd9d0 Compare November 5, 2021 07:27

nitishkumar71 changed the title ~~Include Test case for Limits~~ [WIP] Include Test case for Limits Nov 5, 2021

nitishkumar71 force-pushed the memory_support branch 3 times, most recently from 7e43851 to 3dbb296 Compare November 5, 2021 09:03

nitishkumar71 changed the title ~~[WIP] Include Test case for Limits~~ Include Test case for Limits Nov 5, 2021

LucasRoesler approved these changes Nov 6, 2021

View reviewed changes

alexellis reviewed Nov 10, 2021

View reviewed changes

nitishkumar71 force-pushed the memory_support branch from 3dbb296 to e1be244 Compare November 12, 2021 02:43

nitishkumar71 requested review from LucasRoesler and alexellis November 12, 2021 02:59

nitishkumar71 force-pushed the memory_support branch 2 times, most recently from d098bdc to 453cf64 Compare December 1, 2021 03:36

nitishkumar71 force-pushed the memory_support branch from 453cf64 to dd31ba4 Compare December 5, 2021 17:47

nitishkumar71 force-pushed the memory_support branch from dd31ba4 to 731e6a0 Compare December 16, 2021 02:45

nitishkumar71 force-pushed the memory_support branch from 731e6a0 to b76169b Compare December 24, 2021 05:51

nitishkumar71 force-pushed the memory_support branch from b76169b to c0e539f Compare December 26, 2021 10:01

alexellis reviewed Jan 5, 2022

View reviewed changes

nitishkumar71 force-pushed the memory_support branch from c0e539f to 599952d Compare January 5, 2022 17:46

nitishkumar71 requested a review from alexellis January 5, 2022 17:54

LucasRoesler requested changes Jan 6, 2022

View reviewed changes

nitishkumar71 force-pushed the memory_support branch 2 times, most recently from f02b179 to 573afad Compare January 25, 2022 18:18

nitishkumar71 force-pushed the memory_support branch 2 times, most recently from 00a0232 to daf52e0 Compare March 26, 2022 14:54

nitishkumar71 force-pushed the memory_support branch from daf52e0 to 8d53e82 Compare March 26, 2022 15:00

nitishkumar71 requested a review from LucasRoesler March 26, 2022 15:12

LucasRoesler approved these changes Mar 27, 2022

View reviewed changes

LucasRoesler merged commit 2e39d05 into openfaas:master Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include Test case for Limits #83

Include Test case for Limits #83

nitishkumar71 commented Nov 5, 2021

nitishkumar71 commented Nov 5, 2021

alexellis Nov 10, 2021

nitishkumar71 Nov 12, 2021

nitishkumar71 Dec 5, 2021

alexellis commented Dec 1, 2021

alexellis commented Dec 8, 2021

alexellis commented Dec 8, 2021

nitishkumar71 commented Dec 12, 2021

nitishkumar71 commented Dec 24, 2021 •

edited

Loading

alexellis Jan 5, 2022 •

edited

Loading

nitishkumar71 Jan 5, 2022

LucasRoesler left a comment

LucasRoesler Jan 6, 2022

nitishkumar71 Jan 9, 2022

nitishkumar71 Jan 24, 2022 •

edited

Loading

LucasRoesler Mar 19, 2022

LucasRoesler Mar 19, 2022

nitishkumar71 Mar 26, 2022

LucasRoesler commented Apr 6, 2022

Include Test case for Limits #83

Include Test case for Limits #83

Conversation

nitishkumar71 commented Nov 5, 2021

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

nitishkumar71 commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Dec 1, 2021

alexellis commented Dec 8, 2021

alexellis commented Dec 8, 2021

nitishkumar71 commented Dec 12, 2021

nitishkumar71 commented Dec 24, 2021 • edited Loading

alexellis Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasRoesler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishkumar71 Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasRoesler commented Apr 6, 2022

nitishkumar71 commented Dec 24, 2021 •

edited

Loading

alexellis Jan 5, 2022 •

edited

Loading

nitishkumar71 Jan 24, 2022 •

edited

Loading