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

All functions who creates some resource should be idempotent #120

Open
ppizarro opened this issue May 25, 2017 · 9 comments
Open

All functions who creates some resource should be idempotent #120

ppizarro opened this issue May 25, 2017 · 9 comments

Comments

@ppizarro
Copy link
Contributor

I think a build script should be idempotent. This is expected behavior for most build scripts.

It means, if I run the script again and nothing changed the script should do nothing and generate no error. If only one resource has changed or is a new resource then only this resource should be affected when I run the script again.

KLB could help address this behavior by facilitating your users.

Currently for some resource types when I create the resource and it already exists KLB returns an error for others does not return error. This behavior reflects the behavior of azure cli.

It would be nice if KLB was not just a wrapper to call the commands of the cli.

@katcipis
Copy link
Contributor

It seems that what you desire is consistency on the behavior of the functions when resources already exists. I say this because the behavior already seems to be idempotent, you can call a function repeatedly, it will work the first time and the other times it will fail (or not) but the state of the system will be the same (only one resource will exist on the cloud). Or there is some klb function that creates a duplicated resource ? (I think it is not possible because of Azure behavior of requiring unique names).

Having said that, I completely agree on consistent behavior. @ppizarro could you give some concrete examples on where we can start to work on that ?

@ppizarro
Copy link
Contributor Author

ppizarro commented May 25, 2017

KLB (azure cli) don't create a duplicate resource but it returns an error who stops the script build.

KLB don't help me :(

@katcipis
Copy link
Contributor

damn you @lborguetti why don't you help @ppizarro ? =P

I agree with you @ppizarro lets be consistent. The "crash" on error is useful for a lot of scripts, but for a library it is a terrible behavior, specially because on nash if the crash happens inside a function you have no way to avoid the script to abort (that I know off, of course, perhaps @tiago4orion knows some hack).

I vote on consistency and never aborting. I started to work like that on the backup routines, but most part of klb does not do that, even some stuff that I coded on the past (I was keeping consistency with the major part of the library, that was aborting at the time).

@ppizarro
Copy link
Contributor Author

A concrete example:

	rules = (
		(
		    "deny-all-traffic"
		    "4000"
		    "*"
		    "*"
		    "*"
		    "*"
		    "*"
		    "Deny"
		    "Inbound"
	        )
		(
		    "allow-bastion-host"
		    "3000"
		    "*"
		    $bastion_network_address
		    "*"
		    $address_range
		    "*"
		    "Allow"
		    "Inbound"
	    )
	)

	for rule in $rules {
		nsg <= azure_nsg_rule_new($rule[0], $vnet_resource_group, $nsg_name, $rule[1])
		nsg <= azure_nsg_rule_set_protocol($nsg, $rule[2])
		nsg <= azure_nsg_rule_set_source_address($nsg, $rule[3])
		nsg <= azure_nsg_rule_set_source_port($nsg, $rule[4])
		nsg <= azure_nsg_rule_set_destination_address($nsg, $rule[5])
		nsg <= azure_nsg_rule_set_destination_port($nsg, $rule[6])
		nsg <= azure_nsg_rule_set_access($nsg, $rule[7])
		nsg <= azure_nsg_rule_set_direction($nsg, $rule[8])

		azure_nsg_rule_create($nsg)
	}

The code above built the initial version of my infra.

After it's built I need to add new rules.

I'd like to simply add the new rules to my list of rules and run the script again.

Today, the script stop on first rule with an error.

The KLB could in a first step handle the error. The script needs to know if the error was because it already exists or because the cli azure failed.

In a second step KLB could not return an error if the resource / rule already exists and nothing has changed in it.

Maybe this behavior can be activated by passing a parameter or using another function.

@katcipis
Copy link
Contributor

@ppizarro from what I understand you want to represent 3 states:

  • created
  • failed
  • already exists

We could use an enumeration on the return, like:

result <= func()
if $result == "created" {
}
if $results =="exists" {
}
if $results == "error" {
}

But it makes a little harder to provide a detailed error message. One alternative could be:

created, err := func()
if $err != "" {
    echo "something wrong happened"
}

if $created == "0" {
    echo "success and created"
}

if $created != "0" {
    echo "success, but not created"
}

With booleans it would be even easier. Passing a parameter seems a little odd to me because I'm not sure how the return values change depending on the parameter. If the idea is to the error to be empty or not according to a parameter it seems a little confusing to me.

@ppizarro
Copy link
Contributor Author

ppizarro commented May 25, 2017

I don't undestand this:

if $created != "0" {
    echo "success, but not created"
}

success or failed?

@i4ki
Copy link

i4ki commented May 25, 2017

Problem1: No error handling: https://github.com/NeowayLabs/klb/blob/master/azure/nsg.sh#L87
Problem2: The lack of a helper function to ignore if rule already exists.

The first problem was discussed several times and we all agree that klb should never abort.
For the second problem, we can do even more and validate if the source rules (in the script files) and the server ones (in azure subscription) match, creating the missing rules and aborting with error in case someone added a rule directly by web portal.

@ppizarro As we discussed today, I dont think klb should be only a lib wrapper. I do think we need to create a tool that help us dealing with cloud challenges.

I'd talked with @lborguetti in the past about some experiments I did with aws for a very similar feature. In the case, I've made a tool to query aws instances and generate a instances.sh file,in nash syntax then imported it in my deploy script. I do believe this kind of tooling and helpers should be in klb in the future, but in my understanding now we're still creating the building blocks.

@katcipis
Copy link
Contributor

katcipis commented May 25, 2017

@ppizarro on this context $created would be a boolean indicating if the resource was created or not. You would have an error variable used to indicate if something went wrong (an actual error) and a boolean to indicate in case of success, if it is a success because the resource already exists or not.

In the ideal world, with booleans, it would be:

created, err := func()
if $err != "" {
    echo "something wrong happened"
}

if $created {
    echo "success and created"
} else {
    echo "success, but not created"
}

But we do not have booleans on nash yet, and representing booleans on nash is confusing because we are using "0" as success since this is way processes indicates terminating with success.

This way, if err is "nil" (empty) you can be sure that the resource you requested does exist on the cloud. If you are interested on knowing if it already existed or if it was created you can use the "boolean" $created. Again, confusing because we actually do not have booleans, yet.

If you are just interested on success or not, you can run:

_, err := func()
if $err != "" {
    echo "something wrong happened, resource will not exist on the cloud"
}

echo "success, resource is on the cloud, dont care if it already existed"

@ppizarro
Copy link
Contributor Author

@tiago4orion I agree with you. I'd love if KLB were not just a wrapper!

@katcipis I go it, thanks.

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

No branches or pull requests

3 participants